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.

Sign up for GitHub

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

Jump to bottom

Fix PyroModule rendering error in local-parameter mode #3366

Merged
merged 2 commits into from
May 7, 2024
Merged

Fix PyroModule rendering error in local-parameter mode #3366

merged 2 commits into from
May 7, 2024

Conversation

eb8680
Copy link
Member

@eb8680 eb8680 commented May 7, 2024

Resolves #3365

This PR fixes the bug in pyro.render_model reported in #3365 by adding missing constraint=... keyword arguments to a few dummy param statements inside PyroModule that are invoked when using module_local_params=True. It also adds a unit test that would have caught #3365.

@eb8680 eb8680 requested a review from ordabayevy May 7, 2024 13:53
ordabayevy
ordabayevy approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@ordabayevy ordabayevy left a comment

Choose a reason for hiding this comment

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

Nice catch! I remember when I was reviewing the initial PR you said that those kwargs still might be necessary sometimes even though the lambda function doesn't use them.

@eb8680 eb8680 merged commit 7511353 into dev May 7, 2024
9 checks passed
@eb8680 eb8680 deleted the fix-param-rendering branch May 7, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ordabayevy ordabayevy ordabayevy approved these changes

Assignees
No one assigned
Labels
awaiting review bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rendering PyroModules can fail with local parameter mode enabled
2 participants
@eb8680 @ordabayevy