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

Allow specifying a non-relative module name in config_path #2565

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

simpkins
Copy link
Contributor

Motivation

When constructing the config search path, allow callers to directly pass in a non-relative module name starting with pkg://. This allows callers to bypass the relative name lookup, and always get consistent behavior regardless of the current environment variables at runtime.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Added a new entry in test_config_search_path.py

Related Issues and PRs

#2564

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 31, 2023
@omry
Copy link
Collaborator

omry commented Feb 2, 2023

Nice, thanks.
I will let @Jasha10 accept and merge. I don't know what is the status of the CI.

@simpkins
Copy link
Contributor Author

simpkins commented Feb 3, 2023

The CI failures look like they are because the Netlify config is using an old Ubuntu 16.04 image, which is no longer supported. I'll ping some other folks about trying to get that config fixed.

@omry
Copy link
Collaborator

omry commented Feb 5, 2023

Can you ping the open source team (OSS support IIRC) about it?
I believe the website preview functionality is managed by them.
If they can't fix it I think we should probably just disable the website preview from CI.

When constructing the config search path, allow callers to directly pass
in a non-relative module name starting with `pkg://`.  This allows
callers to bypass the relative name lookup, and always get consistent
behavior regardless of the current environment variables at runtime.

This addresses issue facebookresearch#2564.
@simpkins
Copy link
Contributor Author

simpkins commented Feb 6, 2023

The OSS team says they have fixed the website preview. I've rebased this pull request just to re-trigger the CI checks.

@simpkins
Copy link
Contributor Author

simpkins commented Feb 6, 2023

The CI test failures look like pre-existing problems occurring on main. I sent out #2576 to auto-apply some of the black formatting fixes that are currently causing the tests to fail.

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

Thanks very much @simpkins!

@Jasha10 Jasha10 merged commit 53d07f5 into facebookresearch:main Feb 7, 2023
Jasha10 added a commit to Jasha10/hydra that referenced this pull request Feb 22, 2023
…esearch#2565)

When constructing the config search path, allow callers to directly pass
in a non-relative module name starting with `pkg://`.  This allows
callers to bypass the relative name lookup, and always get consistent
behavior regardless of the current environment variables at runtime.

This addresses issue facebookresearch#2564.

Co-authored-by: Adam Simpkins <[email protected]>
Co-authored-by: Jasha <[email protected]>
Jasha10 added a commit that referenced this pull request Feb 22, 2023
When constructing the config search path, allow callers to directly pass
in a non-relative module name starting with `pkg://`.  This allows
callers to bypass the relative name lookup, and always get consistent
behavior regardless of the current environment variables at runtime.

This addresses issue #2564.

Co-authored-by: Adam Simpkins <[email protected]>
Co-authored-by: Jasha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants