search

justin13601 / ACES

ACES: Automatic Cohort Extraction System for Event-Streams
https://eventstreamaces.readthedocs.io/en/latest/
MIT License
25 stars 1 forks source link

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

Closed mmcdermott closed 2 months ago

mmcdermott commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Yes. I spent literally hours today debugging an issue in this PR https://github.com/mmcdermott/MEDS_transforms/pull/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).

mmcdermott commented 2 months ago

Relevant source hydra issue: https://github.com/facebookresearch/hydra/issues/1828