Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

fix FromParams bug #4841

Merged
merged 3 commits into from
Dec 5, 2020
Merged

fix FromParams bug #4841

merged 3 commits into from
Dec 5, 2020

Conversation

epwalsh
Copy link
Member

@epwalsh epwalsh commented Dec 4, 2020

This was an interesting one. I found this after digging into the test that seemed to fail out of nowhere in this (unrelated) PR: https://github.com/allenai/allennlp-models/pull/175/checks?check_run_id=1499712028. This bug only manifested itself because of a different bug fix regarding the ShardedDatasetReader (#4830).

Basically, the ShardedDatasetReader needs to inspect its **kwargs for the "lazy" parameter. The behavior depends on whether or not "lazy" is present. But the current FromParams logic removes the "lazy" param from kwargs if it's the same as the default value in the superclass.

So this fixes the behavior of FromParams so that parameters in the Params object will ALWAYS be passed to the constructor.

@epwalsh epwalsh requested a review from dirkgr December 4, 2020 17:48
Copy link
Member

@dirkgr dirkgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the full context, so I trust that you've thought through this more than I have. Is there any chance this will break other expected behavior?

@epwalsh
Copy link
Member Author

epwalsh commented Dec 5, 2020

Is there any chance this will break other expected behavior?

I can't be completely certain, but we do have very high test coverage here that covers every edge case I can think of so far.

@epwalsh epwalsh merged commit 63b6d16 into master Dec 5, 2020
@epwalsh epwalsh deleted the from-params-bug branch December 5, 2020 06:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants