Conversation
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
3ab2394 to
41cde8b
Compare
Deploying databuilder-docs with
|
| Latest commit: |
e142f71
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3ef49cdc.databuilder.pages.dev |
| Branch Preview URL: | https://evansd-refactor-loaders.databuilder.pages.dev |
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
41cde8b to
15fbffe
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
15fbffe to
1f242d0
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
1f242d0 to
7609b06
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
7609b06 to
3da0591
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
3da0591 to
67cee40
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
67cee40 to
cef1a6c
Compare
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
cef1a6c to
0fd99bc
Compare
rebkwok
approved these changes
Mar 31, 2026
This pulls a generic `run_ehrql_command_in_subprocess()` function out of the more specific `load_definition_in_subprocess()` function.
We're not trying to retrieve a value when running `debug`: we just want to execute the Python and collect any output. Making this a separate function removes special-casing elsewhere.
We can also simplify the argument handling here by using `choices`.
This is designed to represent all the various values we may be interested in from a user-supplied definition module. We need to support serializing errors because the pre-serialization code will no longer know which kinds of error the calling code cares about.
Instead of having specialised loaders for each definition type we now have a single function which loads the module, calls various `populate_*_details` functions to grab potentailly relevant attributes from the module, and returns a `ModuleDetails` object which collects all these attributes. This `ModuleDetails` object can then be serialized, passed back to the parent process and it can then decide what details it cares about. Note that we now return exceptions rather than raising them immediately. For example, if we are loading measure definitions we don't care if `dataset` doesn't have a population defined yet. But we can't just go ahead and serialize it because it's not possible to construct a serialized dataset without a population definition. So we return the error and let the caller decide whether to raise it or not. To make the change easier to read we continue passing around now useless `definition_type` arguments. We'll remove these in a later commit.
The serializer, and functions upstream of it, no longer care what type of definition module they're working with.
The old name made it sound like is was loading a particular "debug" type of definition, whereas it's not really loading anything (as it doesn't return a value) it's running an arbitrary definition in a particular mode.
evansd
force-pushed
the
evansd/refactor-loaders
branch
from
0fd99bc to
e142f71
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refactors some of the code for loading user-supplied modules to use a single
ModuleDetailstype to represent all the values we might care about in one of these modules, including potential errors. The aim is to end up with more consistent handling of these such that we can have more features that can work with either datasets or measures (see #2313).We recently added measures support to
create-dummy-tablesbut this was made unnecessarily difficult by the way loaders were implemented. This refactoring aims to change that.We also treat running user-supplied modules in "debug" mode as a separate task to serializing their contents which removes some incidental complexity.