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

Features/sg 409 check all params used #546

Merged
merged 13 commits into from
Dec 11, 2022

Conversation

BloodAxe
Copy link
Contributor

@BloodAxe BloodAxe commented Dec 6, 2022

No description provided.

@dagshub
Copy link

dagshub bot commented Dec 6, 2022

Copy link
Contributor

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

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

Looks really good!

My only concern, is for the DictConfigs we get back from hydra_instantiate, which we use when we launch recipe: in the unit tests we use OmegaConf.create() to mimic this scenario. Can you confirm their contents would be similar?
This way or the other, I think it'd be wise to instantiate one of our yaml files (our create one for the sake of testing) and test this out.

@BloodAxe BloodAxe marked this pull request as ready for review December 7, 2022 10:28
@BloodAxe BloodAxe requested a review from ofrimasad as a code owner December 7, 2022 10:28
@BloodAxe
Copy link
Contributor Author

BloodAxe commented Dec 7, 2022

@shaydeci I've added a test that checks instantiation of a model from YAML file (One of just a few that can be used as is without bells & whistles) which pass nicely. Checking all YAMLs in this way is a subject for another ticket: https://deci.atlassian.net/browse/SG-498

    def test_resnet18_cifar_arch_params(self):
        arch_params = get_arch_params("resnet18_cifar_arch_params")
        architecture_cls, arch_params, pretrained_weights_path, is_remote = get_architecture("resnet18", HpmStruct(**arch_params))

        with raise_if_unused_params(arch_params) as tracked_arch_params:
            _ = architecture_cls(arch_params=tracked_arch_params)

        with self.assertRaisesRegex(UnusedConfigParamException, "Detected unused parameters in configuration object that were not consumed by caller"):
            arch_params.override(me_is_not_used=True)
            with raise_if_unused_params(arch_params) as tracked_arch_params:
                _ = architecture_cls(arch_params=tracked_arch_params)

Copy link
Contributor

@shaydeci shaydeci left a comment

Choose a reason for hiding this comment

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

LGTM

@ofrimasad ofrimasad merged commit 0ecabe6 into master Dec 11, 2022
@ofrimasad ofrimasad deleted the features/SG-409-check-all-params-used branch December 11, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants