Skip to content
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

Checking if the parameters are a DictConfig Object #2216

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

ssakhavi
Copy link
Contributor

#614
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?

Fixes #2058 .

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.

To be honest, I have no idea how I should go about writing a test for this.

@mergify mergify bot requested a review from a team June 17, 2020 03:24
@Borda Borda added bug Something isn't working feature Is an improvement or enhancement labels Jun 17, 2020
pytorch_lightning/loggers/base.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team June 17, 2020 06:23
@ssakhavi
Copy link
Contributor Author

@Borda
The change you proposed caused some of the tests to not pass. Do you know the reason?

@Borda
Copy link
Member

Borda commented Jun 17, 2020

The change you proposed caused some of the tests to not pass. Do you know the reason?

you need to propagate the change in your code, like test Container instead of DictConfig

@Borda Borda added this to the 0.8.x milestone Jun 19, 2020
@Borda
Copy link
Member

Borda commented Jun 19, 2020

in fact, the solution shall be that DictConfig is inherited from dict, so no change needed for PL

ssakhavi and others added 3 commits June 19, 2020 20:44
This is in reference to Lightning-AI#2058 . 

To be honest, I have no idea how I should go about writing a test for this.
@Borda
Copy link
Member

Borda commented Jun 19, 2020

cc: @omry @williamFalcon

@mergify mergify bot requested a review from a team June 19, 2020 18:48
@Borda Borda added the ready PRs ready to be merged label Jun 19, 2020
@ssakhavi
Copy link
Contributor Author

A question:

If DictConfig inherits from Dict and the isinstance method wasn't working in the crashing version, why should the MutableMapping work now? Wouldn't it be the same situation?

@omry
Copy link
Contributor

omry commented Jun 21, 2020

A question:

If DictConfig inherits from Dict and the isinstance method wasn't working in the crashing version, why should the MutableMapping work now? Wouldn't it be the same situation?

DictConfig inherits from MutableMapping as of OmegaConf 2.0 which comes the Hydra 1.0 release candidate.
If you are using Hydra 0.11.3 you should switch to Hydra 1.0.0rc1 (or newer one they are released) for this to work.

@mergify mergify bot requested a review from a team June 23, 2020 13:28
@Borda Borda merged commit 44385bb into Lightning-AI:master Jun 23, 2020
@ssakhavi ssakhavi deleted the patch-1 branch June 28, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hydra MLFlow Clash
5 participants