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

[RFC] Organizational structure for Reich Lab models and their materials #16

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lshandross
Copy link

@elray1 and I were discussing where to put the files from a retrospective analysis of a potential model for FluSight, then realized having a standard organizational structure for modeling materials would be useful

Deadline for comments: Tuesday, January 21 (because of the holiday)

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

As a non-modeler but organization enthusiast, this looks good to me.


### 2 `operational-models` repository

We will use [`operational-models`](https://github.com/reichlab/operational-models) to store scripts containing code to run a model once, for routine weekly submissions (including the creation of some plots). This repository will contain such scripts for ALL of the Lab's models submitting to a forecast hub, separated into model-specific directories following the naming convention `disease_modelname`.
Copy link
Contributor

@bsweger bsweger Jan 21, 2025

Choose a reason for hiding this comment

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

I haven't been following along too closely, so apologies for the question about something that's already been discussed.

From the perspective of someone who hasn't been hands-on with the current setup (i.e., doesn't have a full understanding of the challenges involved), it seems like the the models and the operational-models repo are very tightly-coupled.

For example (if my quick read is correct), operational-models is responsible for building models' Docker images and exposing the run.sh command. That seems like a concern of the model repos. [edited to clarify: ideally, changing a model's requirements, parameters, or run incantations would happen in the model's repo without also needing to update a centralized operational-models repo]

That said, am channeling my inner @zkamvar .... if the process as outlined is working, we can always create another RFC if we want to migrate to a more decentralized process.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by the "model repos" — do you mean the package repository or the evaluations/experiments repository (or something else)?

If you mean the package repository, from what I understand the requirements, parameters, or run incantations don't belong there. There might be some argument that those things could live in the evaluation/experiments repo, but I think that they are best suited for the scripts within the operational-models repo, which is responsible for all business as usual model runs. And the requirements, parameters, and run incantations generally won't live anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you mean the package repository, from what I understand the requirements, parameters, or run incantations don't belong there.

If by "package repository," you mean the repo that contains the actual code for the model, then yeah, that's what I meant.

I don't understand why the requirements, parameters, or run incantations don't belong there. Are we saying that our shared models can only ever be run via operational-models as an intermediary?

If our goal is to publish sharable models, separating the model code form its build incantations (e.g., Dockerfile) and from the cli/APi/whatever that runs the model seems counterintuitive (@matthewcornell that's what I meant by separation of concerns).

All that said, I'm not an machine learning ops practitioner, so it's entirely possible I don't have the right context for this opinion!

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I'm not sure I have the answers to your questions which are very valid. Maybe @elray1 could help explain further?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think Becky's suggestion is reasonable, though I think I'd put the operational code under point 4 below.

At the same time, I'm not eager to make work for ourselves by shuffling things around. Maybe we could add these other possibilities in the "other options considered" section.

@matthewcornell matthewcornell self-requested a review January 21, 2025 19:14
Copy link
Member

@matthewcornell matthewcornell left a comment

Choose a reason for hiding this comment

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

This looks good to me. I didn't understand @bsweger 's comment about separation of concerns, though...

Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

had a couple of minor comments, but it looks good. thanks!


We will use this repository to store materials that fall under the following purposes: (1) archived submission files or (2) evaluations (retrospective or otherwise) and experiments.

Archived submission files include the actual model outputs we submit to the target hub repo (with any invalid or particularly poor predictions removed), the raw version of those model outputs (with no predictions removed), and the associated plots of the raw model outputs (which are also sent to Slack for review).
Copy link
Contributor

Choose a reason for hiding this comment

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

the actual model outputs we submit to the target hub repo (with any invalid or particularly poor predictions removed), the raw version of those model outputs (with no predictions removed)

This seems like a reasonable idea, but is not something we always do right now. Is it worth updating our existing procedures to archive these things? Do we have a use for them?

and the associated plots of the raw model outputs (which are also sent to Slack for review).

I think I'd rather not commit ourselves to always dropping these pdfs of plots into github. I basically would never look at these plots again, and they can be bulky. We could remove this part, or just say "optionally" here, up to the model developer as they see fit?

Copy link
Author

Choose a reason for hiding this comment

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

  1. I just remembered that we had saved at least a local copy of raw model outputs when removing certain forecasts, so I thought it might be good to include this as standard practice. But I don't have super strong feelings on it, and we could just make it optional (with a strong suggestion to do so if more than n forecasts are removed)
  2. I'm fine with making storage of plots optional.

Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Copy link
Member

@nickreich nickreich left a comment

Choose a reason for hiding this comment

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

I have one small question about clarifying what the code does in the evals/experiments repo, but basically I approve of this as is. It's a serious upgrade over our current documentation for this (which is nil).


Archived submission files include the actual model outputs we submit to the target hub repo (with any invalid or particularly poor predictions removed), the raw version of those model outputs (with no predictions removed), and the associated plots of the raw model outputs (which are also sent to Slack for review).

Evaluations and experiments include code to run a model repeatedly (i.e., several past weeks) as part of a retrospective evaluation, model outputs for multiple retrospective model runs, code to systematically evaluate model outputs, and results of running code to evaluate model outputs, e.g. plots and scores.
Copy link
Member

Choose a reason for hiding this comment

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

I have a question somewhat similar to @bsweger 's question above. Could this document make more clear what kind of code lives in this "evals and experiments" repo? E.g. there is code in this repo that will load/call the model from the "R or Python package repo". And it may have some overlapping/duplicative code to that in "operational-models", but it will be different from "operational-models" in that the code will be used in the context of generating submission files for retrospective experiments?

Copy link
Author

Choose a reason for hiding this comment

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

How does the below sound?

Evaluations and experiments include code to run a model repeatedly (i.e., several past weeks) using the package from the Package Repository as part of a retrospective evaluation, model outputs for multiple retrospective model runs, code to systematically evaluate model outputs, and results of running code to evaluate model outputs, e.g. plots and scores. Note that the code from retrospective model runs functions to generate submissions for multiple rounds simultaneously, which is the key difference from the scripts stored in the operational-models repo even if there are similarities between the two code bases.

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.

6 participants