args should come after the last positional argument

[yukw777]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Because *args came after the last kwarg in load_from_checkpoint, you couldn't pass in positional arguments correctly to the constructor of your model. This fixes that.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #1807 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1807   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          71      71           
  Lines        4431    4431           
======================================
  Hits         3893    3893           
  Misses        538     538           

[mergify]

This pull request is now in conflict... :(

[festeh]

+1. Recently faced this issue, it would be nice to have this behavior:

class Module(...):
    def __init__(self, hparams, param1, param2, param3):
        ...

param1 = ...
param2 = ...
param3 = ...

ckpt_path = ...
my_module = Module.load_from_checkpoint(ckpt_path, param1, param2, param3)

On the other hand, it'll break existing code that passes map_location as a positional argument. Still, I think this change makes loading process easier

[williamFalcon]

@Borda let's add this to 0.7.6? need to get these tests to pass

[awaelchli]

unfortunately saw this too late but docs were not updated.
Docs should reflect changes like this, especially when they break backwards compat.
@yukw777 would you be willing to make a follow up?
I am also surprised the tests don't fail, especially because this is a breaking change. Or do I miss something?

[yukw777]

@awaelchli I couldn't find any test cases for this, so I just left it at that. 🤷‍♂️ I didn't think to update it b/c no one could really use *args at all before this change, so this fix would actually make the code consistent with the doc we have now. As @festeh said, some code may break, but I didn't think this would be a big issue, b/c that is a wrong way to call this method anyway, mixing args and kwargs like that. Having said that, I'm happy to send you a PR to fix the doc. Can you advise me on which doc I should update?

[awaelchli]

ok, got it. yes if it could not be used before then I understand why there is no test.
I think now that it is working properly, we should have a test that calls the load function with at least one arg and one kwarg and check that the model is correctly initialized.
In the docs, I saw there is just a *model_args but no kwargs. The docs are in the header of the function declaration, so we can directly update it there.