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

We should make our config names more private to this package to avoid hydra name collisions. #122

Closed
mmcdermott opened this issue Aug 29, 2024 · 3 comments · Fixed by #123

Comments

@mmcdermott
Copy link
Collaborator

E.g., add "_"s in front of them. This won't affect our code's interface, but if someone ever tries to import one of our provided configs as a default in hydra, this will make it harder to accidentally suffer from name collision which causes hydra to go into an infinite recursion loop.

@justin13601
Copy link
Owner

E.g., add "_"s in front of them. This won't affect our code's interface, but if someone ever tries to import one of our provided configs as a default in hydra, this will make it harder to accidentally suffer from name collision which causes hydra to go into an infinite recursion loop.

Do you mean _aces.yaml, data/_sharded.yaml, data/_single_file.yaml etc.?

@mmcdermott
Copy link
Collaborator Author

Yes. I spent literally hours today debugging an issue in this PR mmcdermott/MEDS_transforms#187 because my "pipeline.yaml" configuration file in a test had the same name as a "pipeline.yaml" config in MEDS_Transforms that was imported via hydra as a default and it was really hard to debug.

For MEDS_transforms, this is problematic as if you write your file to disk somewhere, you are likely tempted to call it something like "pipeline" or "preprocess" or "extract" (all the default config names for that package). So, I moved all the configs inside the package to "_pipeline", "_preprocess", "_extract" and it should help avoid this issue. We may have the same issue with aces, so should do the same here.

Except, I'd only advocate for this with aces.yaml. Not with data/_s* b/c we need those names to match the option names in how you use them (e.g., data=sharded).

justin13601 added a commit that referenced this issue Aug 29, 2024
@mmcdermott
Copy link
Collaborator Author

Relevant source hydra issue: facebookresearch/hydra#1828

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 a pull request may close this issue.

2 participants