-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable custom default config for dataset tests #3578
Conversation
I've included some other documentation upgrades. If you want these in separate PRs, let me know. |
Codecov Report
@@ Coverage Diff @@
## master #3578 +/- ##
==========================================
- Coverage 79.07% 79.07% -0.01%
==========================================
Files 105 105
Lines 9787 9786 -1
Branches 1572 1572
==========================================
- Hits 7739 7738 -1
Misses 1567 1567
Partials 481 481
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmeier , I made a few comments. I find the logic a bit tricky to follow overall, as the OO implementation is very stateful. It might not be worth considering a refactoring, but I think at least DEFAULT_CONFIG
could be renamed in something more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some confusion about how the configs are handled. I hope I can make it more clear with my comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these precisions @pmeier , I made a second pass
Following our internal discussion, I refactored a little more:
Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pmeier , some last comments but LGTM!
@NicolasHug I've addressed all your comments. Thanks a lot for the input! IMO it made the PR a lot better 🙂 Merge if you and the CI are happy. |
Thanks @pmeier ! |
Reviewed By: fmassa Differential Revision: D27433915 fbshipit-source-id: 70d5fcd0a8b68c2de7362ddf4f63a072cf658d7c
This came up in #3423 (comment).
TL;DR: With the current architecture, it is not possible to test a dataset that has a positional parameter, which should change depending on the config.
To overcome this, this PR makes the
DEFAULT_CONFIG
a first class member. It still defaults to the keyword arguments extracted from the dataset constructor, but can now be safely overwritten if needed.