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

[Bug] relative config_path resolution is not consistent #2564

Open
2 tasks done
simpkins opened this issue Jan 31, 2023 · 1 comment
Open
2 tasks done

[Bug] relative config_path resolution is not consistent #2564

simpkins opened this issue Jan 31, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@simpkins
Copy link
Contributor

🐛 Bug

Description

The config_path parameter to hydra.main() and hydra.experimental.initialize() must be a path name, or a relative module name.

If a relative module name is passed in, hydra does not behave consistently about what this module name is relative to. If any of HYDRA_MAIN_MODULE, FB_PAR_MAIN_MODULE, or FB_XAR_MAIN_MODULE is set, the argument is treated relative to the value of that environment variable. Otherwise hydra tries to determine the module that contains the task function, and the config_path is treated relative to that module.

This results in different behavior based on environment variables. In our code base this has resulted in hundreds of locations where users are manually checking these environment variables before calling hydra.main(), and passing in different config paths based on the value of these environment variables. (or, in a handful of cases, unsetting all of these variables before calling hydra.) It seems better to simply fix hydra to behave consistently rather than having hundreds of call sites working around the hydra behavior.

Note that in our situation this mainly matters for FB_XAR_MAIN_MODULE and FB_PAR_MAIN_MODULE, since these environment variables are set conditionally depending on the current build mode, so hydra behaves differently in different build modes.

To minimize the impact of any code depending on the current behavior, it would probably be easiest to provide some way for callers to pass in a top-level module path, and prevent hydra from treating it as relative. e.g., we could perhaps allow config_path to start with pkg:// to indicate that what follows is a top-level module name.

#2395 added support for absolute paths, but there is no mechanism to pass in non-relative modules that I can tell.

Checklist

  • I checked on the latest version of Hydra
  • I created a minimal repro (See this for tips).

Expected Behavior

Provide a consistent behavior for config_path that does not depend on build mode.

System information

  • Hydra Version : 1.3.1
  • Python version : 3.8.6
@simpkins simpkins added the bug Something isn't working label Jan 31, 2023
simpkins pushed a commit to simpkins/hydra that referenced this issue Jan 31, 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 facebookresearch#2564.
simpkins pushed a commit to simpkins/hydra that referenced this issue Jan 31, 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 facebookresearch#2564.
simpkins pushed a commit to simpkins/hydra that referenced this issue Feb 6, 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 facebookresearch#2564.
Jasha10 added a commit that referenced this issue Feb 7, 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]>
Jasha10 added a commit to Jasha10/hydra that referenced this issue 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 issue 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]>
@omry
Copy link
Collaborator

omry commented Feb 27, 2023

The automatic detection of the main module is a tricky part of Hydra.
It was not possible in fbcode for some reason (at least, I was not able to support it in fbcode in the same way). This led me to add support for FB_PAR_MAIN_MODULE and FB_XAR_MAIN_MODULE (HYDRA_MAIN_MODULE is there for a non-fbcode specific solution, but it's not documented, and I don't believe is being used anywhere outside of tests).

Off the top of my head, those environment variables are short-circuiting the detection of the main module and are always treated as absolute modules.

Note that those were meant to be used primarily to detect the main module when the app is running from a package built in fbcode (xar, par).
Setting them in the code is abuse and was never meant to be how Hydra is used. In fact, the whole line of problems you are trying to solve stem from abuse by the hpc team, which resulted in lots of boilerplate code being copied around.
Had they listened to my suggestions early on they would have had less shit to clean up now.
Sorry you got caught in it.

Please explain what inconsistency you are seeing in more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants