warn user when dropping unpicklable hparams

[monney]

What does this PR do?

PR changes namespace cleaning to be more general. Makes sure it only removes items which can't be pickled, and warns user when doing so.

Fixes #2676
Fixes #2444

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

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 🙃

[pep8speaks]

Hello @monney! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-11 18:44:01 UTC

[awaelchli]

@monney I updated your pr descripion. Since you added a warning, I believe this also solves a the issue #2444.

[awaelchli]

@monney is this wip or ready to review? at this point I would suggest to extract a helper function is_pickleable(obj: object) -> bool to avoid duplicated code as it is useful in other places too. Thanks for your efforts.

EDIT: oops just saw tests are failing, I will check what's going on.

[monney]

@awaelchli Should be good to go now. Build is failing, but I believe the test case is broken. Should be checking for nn.CosineEmbeddingLoss but is checking for nn.CrossEntropyLoss.

[awaelchli]

@monney I can confirm, test case needs to be fixed by checking for CosineEmbeddingLoss. While debugging this I wrote a test case for your bugfix, could you add it, maybe below test_hparams_pickle?

class UnpickleableArgsEvalModel(EvalModelTemplate):
    """ A model that has an attribute that cannot be pickled. """

    def __init__(self, foo='bar', pickle_me=(lambda x: x + 1), **kwargs):
        super().__init__(**kwargs)
        assert not is_picklable(pickle_me)
        self.save_hyperparameters()


def test_hparams_pickle_warning(tmpdir):
    model = UnpickleableArgsEvalModel()
    trainer = Trainer(default_root_dir=tmpdir, max_steps=1)
    with pytest.warns(UserWarning, match="attribute 'pickle_me' removed from hparams because it cannot be pickled"):
        trainer.fit(model)
    assert 'pickle_me' not in model.hparams

With this test case I think the PR is ready 👍

[monney]

@awaelchli I added your test case and changed the warning to rank_zero_warn, I think everything should be good to go now!

[awaelchli]

@monney There are test failures, they must be because of the new way of cleaning the namespace, but not sure what needs to be changed. Have you seen it?

[monney]

@awaelchli it is because of the new way of cleaning. Something prevented that test case either from being run or failing previously. The property that it’s checking for would have been completely removed by the old way of cleaning, I’m not sure why that did not trigger a failure though. The new way causes it to fail because it should be checking for CosineEmbedding.

[awaelchli]

Ah I thought I already confirmed it. Yes, please change to CosineEmbeddingLoss I agree.

[monney]

You did. That’s my bad. Doesn’t it need to be changed in the CI server? Or if I change it in my code will it update

[awaelchli]

It's in the same file you added the test, above in a fuction called test_collect_init_arguments. You change it there, push your commit and it will show here in the tab for changed files :)

[monney]

I got it now, I didn't realize it ran the test cases, from the commit itself, and thought you had update it elsewhere. Thank you! :)

[codecov]

Codecov Report

Merging #2874 into master will decrease coverage by 1%.
The diff coverage is 92%.

@@           Coverage Diff            @@
##           master   #2874     +/-   ##
========================================
- Coverage      90%     89%     -1%     
========================================
  Files          79      81      +2     
  Lines        7226    9204   +1978     
========================================
+ Hits         6519    8235   +1716     
- Misses        707     969    +262     

[awaelchli]

@monney thanks for the fix

[nateraw]

LGTM 🚀

[SkafteNicki]

LGTM 👍

[Borda]

this can be quite a time costly especially for large objects or calling this function quite often...