CDCgov / cfa-epinow2-pipeline

https://cdcgov.github.io/cfa-epinow2-pipeline/
Apache License 2.0
10 stars 2 forks source link

Should we do config validation in this pipeline? #78

Open zsusswein opened 1 month ago

zsusswein commented 1 month ago

Months ago, I included config validation on load in #7. In the meantime, @amondal2 wrote https://github.com/CDCgov/cfa-config-validation. Now I'm not sure how we should handle validation in this piece of the pipeline.

An additional consideration: it's going to be a bit of a pain for development to have mandatory config validation. It prevents "I know what I'm doing" overrides of the validation when trying to quickly out new functionality.

Potential options:

  1. Keep validation here as-is. Validation is important (and config errors are one of our main sources of failure). Two validations is better than one.
  2. Keep validation here but point to the microservice. Single source of truth
  3. Don't validate here and assume the user has called the microservice as part of their deployment. Separation of concerns.

Thoughts? @amondal2 @natemcintosh @athowes @kgostic

amondal2 commented 1 month ago

I would lean toward #3 here -- validation should definitely be done for any workflows that are touching production data/deployments/runs, but for development we can call the service manually as needed. We can also call the service at the PR-level so that local development isn't hindered by the validation step, but it does occur before any changes are pushed.

natemcintosh commented 1 month ago

I agree that 3 would probably be good, we just need a good way to call the microservice during the deployment process. Maybe one of:

  1. The kickoff script, wherever that lives, calls it
  2. The config generator calls it immediately after generating a config

One question I have is what exactly does the flow of kicking off runs look like? How do configs get generated in local and remote runs? How do they get handed to Azure? I think that process will have a large impact on how we validate the config.

athowes commented 1 month ago

I don't have a strong opinion but config validation is likely to be required for all models (that use configs) so I'm in favour of it being modular and the parts which are not EpiNow2 specific being outside of this repo (and in cfa-config-validation). And that sounds like option 3.

I'm also in favour of things which make it easier to do development in this repo, and had previous concerns that the config set-up tied hands too much in that regard. Perhaps moving the validation as being more optional helps a little with that, perhaps it doesn't.

zsusswein commented 1 month ago

Sounds like we have a quorum