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

Import job-runner's parsing #32

Merged
merged 26 commits into from
May 24, 2022
Merged

Import job-runner's parsing #32

merged 26 commits into from
May 24, 2022

Conversation

ghickman
Copy link
Contributor

This pulls in project.py (as legacy.py) from job-runner and refactors it into the pydantic models. All validation is currently handled with pydantic (although, as per #31 I don't think that's the ideal situation), with the generation of ActionSpecifications (and thus mutation of the run command for that) left to that method.

I've hopefully made reviewable commits, so they're the possible to follow.

Ref opensafely-core/job-runner#366

@ghickman ghickman force-pushed the support-job-runner branch 2 times, most recently from f19047b to 0ae56ad Compare April 11, 2022 15:09
@iaindillingham iaindillingham self-assigned this Apr 13, 2022
permitted_privacy_levels = [
"highly_sensitive",
"moderately_sensitive",
"minimally_sensitive",
Copy link
Member

Choose a reason for hiding this comment

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

I had no idea there even was a minimally sensitive level!

Copy link
Member

@iaindillingham iaindillingham left a comment

Choose a reason for hiding this comment

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

I've read commit-by-commit and (I think) I understand what you're doing and why you're doing it. I don't have enough experience of job-runner to check that the refactoring hasn't missed something; but nevertheless, everything seems correct. More 👀 needed 🙂

@iaindillingham iaindillingham removed their assignment Apr 13, 2022
@ghickman ghickman force-pushed the support-job-runner branch 2 times, most recently from 7d36f70 to 35eb00d Compare April 26, 2022 09:40
ghickman added 22 commits May 16, 2022 11:12
This uses the pydantic models to load and validate a given project.yaml,
but keeps all the job-runner functions using dicts.  There is duplicate
validation which will be removed in favour of pydantic model validation
in a subsequent change.
We manually parse the run command to build up Command instances so we
also have to handle basic validation like this.
load_pipeline is now a full replacement for it.
@ghickman ghickman force-pushed the support-job-runner branch 3 times, most recently from 8b6e671 to f6126f1 Compare May 20, 2022 10:19
Copy link
Member

@bloodearnest bloodearnest left a comment

Choose a reason for hiding this comment

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

OK, I think we're there! \o/ :superhero:

One question about a possible improvement.

Overall, I'm not convinced the benefits of static typing are worth the extra complexity, but if there was ever a project that this should apply, it's this one.

@ghickman ghickman force-pushed the support-job-runner branch from 7392e4b to 5c03b11 Compare May 24, 2022 12:03
ghickman added 4 commits May 24, 2022 14:44
job-runner needs to mutate the arguments passed to extraction commands
(eg cohortextractor and databuilder) based on pipeline config.  However
we don't think that should be exposed to other users of the library so
now we have boundary where this library is concerned with parsing and
validating the config.
We need various forms of the `run` key a config provides so instead of
library consumers creating these themselves we now only store the raw
version (renamed from run to raw to avoid run.run) and provide
properties for the other forms.
Rather than pulling in is_extraction_command from job-runner this allows
us a lookup table of validation functions which dispatch on the specific
image and command.
@ghickman ghickman force-pushed the support-job-runner branch from 5c03b11 to 40fee6e Compare May 24, 2022 13:44
@ghickman ghickman merged commit 9295ad7 into main May 24, 2022
@ghickman ghickman deleted the support-job-runner branch May 24, 2022 13:46
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