Skip to content

replace SimpleNamespace configs with typed dataclasses and enums#43

Open
matthewcornell wants to merge 3 commits intomainfrom
mc/simple-namespace-refactor
Open

replace SimpleNamespace configs with typed dataclasses and enums#43
matthewcornell wants to merge 3 commits intomainfrom
mc/simple-namespace-refactor

Conversation

@matthewcornell
Copy link
Member

This PR is part of Operational models refactoring ideas #42. It is paired with the idmodels PR [replace SimpleNamespace configs with typed dataclasses and enums #24].

Changes:

  • Replace SimpleNamespace configs with typed dataclasses and enums from idmodels.config across all Python models.
  • Update idmodels to de6be08.
  • Switch Python dependency management from ad-hoc requirements.txt to pip-compile workflow with requirements.in source files; pin Python 3.12 via .python-version.
  • Update README to document the new pip-tools workflow.

… idmodels.config across all Python models. Update idmodels to de6be08. Switch Python dependency management from ad-hoc requirements.txt to pip-compile workflow with requirements.in source files; pin Python 3.12 via .python-version. Update README to document the new pip-tools workflow.
Copy link
Contributor

@lshandross lshandross left a comment

Choose a reason for hiding this comment

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

The changes themselves LGTM, but I was wondering when you were planning to rebuild the docker containers and if we should wait to do so until after forecasts are submitted for this week so we have time to try running the new version before we submit the next round of forecasts.

@matthewcornell
Copy link
Member Author

The changes themselves LGTM, but I was wondering when you were planning to rebuild the docker containers and if we should wait to do so until after forecasts are submitted for this week so we have time to try running the new version before we submit the next round of forecasts.

@lshandross Great question. I was planning on asking everyone's opinion re: when to deploy the changes. I've tested them locally - both direct main.py calls and local docker build and call - and they're all looking good. That said, functionality hasn't changed yet, so I thought we might defer deployment until after we add more refactorings...

@lshandross
Copy link
Contributor

I'm fine with the current plan of delaying (since that's what we did when there were changes in the pipeline before), but we can also discuss during our meeting about both PRs

@matthewcornell
Copy link
Member Author

matthewcornell commented Feb 24, 2026

Thanks for your comments @lshandross and @trobacker . Here's what I distilled from them:

PLAN:

  • class Disease(str, Enum): add RSV
  • class PowerTransform(str, Enum): add NONE
  • class SARIXRunConfig(RunConfig):
    • move these parameters to the model class?
    • eliminate SARIX and GBQR run config classes if possible
  • we may end up having a lot of enums in the configs
    • I think we should keep some kind of a doc for their purpose and intention. E.g. what does "Pooling" mean to a user of the model (even if it may be obvious).
    • documentation on some of the enums within the classes for users?
    • maybe factor in nick's Feature Taxonomy for ML Models
  • remove commented code: # model_config = ...

[new via Nick's comment]:

  • move the inference/fitting parameters (num_warmup, num_samples, num_chains for SARIX; num_bags, bag_frac_samples for GBQR) from RunConfig to ModelConfig
    • drop SARIXRunConfig and GBQRRunConfig entirely and have a single RunConfig class
  • formalize short_run to be a first class feature of model config
    • consider approach outlined in Nick's comment ("put production defaults on the ModelConfig fields, and add a short_run() method that each model subclass defines")

…Config, moving instance variables to corresponding *ModelConfig classes. made RunConfig concrete
@matthewcornell
Copy link
Member Author

I've updated the code to match requested idmodels changes. Please review 4c01eac .

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 this pull request may close these issues.

3 participants