Change api and datamodel to accomodate sequential runs #649
Conversation
Co-authored-by: Zohar Malamant <git@wah.pink> Co-authored-by: Lars Petter Hauge <lars-petter-hauge@users.noreply.github.com>
UsamaEquinorAFK
force-pushed
the
chainrefactor
branch
from
18bd60e to
e31dac4
Compare
UsamaEquinorAFK
force-pushed
the
chainrefactor
branch
from
e31dac4 to
b9d0852
Compare
| } | ||
|
|
||
| if not hasattr(self, "_concentrations"): | ||
| self.concentrations = {} |
| if adapter.category != "Primary": | ||
| adapter.concentrations = { | ||
| k: v | ||
| for k, v in concentrations.items() | ||
| if k in adapter.valid_substances | ||
| } |
There was a problem hiding this comment.
This business logic relating to the adapter doesn't need to be in this file. BaseAdapter should instead have:
validate_concentrationswhich throws an exceptionset_concentrationswhich does this
Remove the setter property.
| raise e | ||
|
|
||
| try: | ||
| adapters[0].concentrations = create_simulation.concentrations |
There was a problem hiding this comment.
Move this up before creating the simulation row in db. No need to schedule a simulation that we know will fail.
| } | ||
|
|
||
|
|
||
| def test_chain(client, dummy_model): |
There was a problem hiding this comment.
Missing tests for the following cases:
- Empty case
- Single model
- Different models
- Parameters passed to the correct model
- What happens if you run 2 models, but first model throws an exception? Does the
/resultcorrectly show that the simulation is no longer running?
| [model_input.id for model_input in simulation.model_inputs], | ||
| ) | ||
| except BaseException as e: | ||
| raise e |
There was a problem hiding this comment.
This try-catch doesn't seem to do anything interesting.
| ModelResult( | ||
| concentrations=db_result.concentrations, | ||
| panels=[ | ||
| TypeAdapter(AnyPanel).validate_python(panel) |
There was a problem hiding this comment.
Why use TypeAdapter? panel is already a pydantic.BaseModel. Does this code ensure that the effect of this is the same as converting all the panels to JSON, including the changing from snake_case to camelCase?
| if result is None: | ||
| return RunResponse(status="pending", model_input=model_input) | ||
| elif result.error is not None: | ||
| db_results = [mi.result for mi in simulation.model_inputs] |
There was a problem hiding this comment.
Do we know that the order of the results is correct? If you submit arcs -> solubility, could you get the results reversed: [result_of_solubility, result_of_arcs]? If yes, how do we avoid this; if not, why not?
| } | ||
| else: | ||
| adapter.concentrations = concentrations | ||
| except InputError: |
There was a problem hiding this comment.
When do we expect there to be an InputError? Are there any other exceptions that may occur here, if any?
| "results", | ||
| sa.Column("simulation_id", sa.UUID(), autoincrement=False, nullable=False), | ||
| ) | ||
| op.drop_constraint(None, "results", type_="foreignkey") # type: ignore # Alembic accepts contraint name as string or None |
There was a problem hiding this comment.
Remove this line. Comment is incorrect: The constraint name must be a str and cannot be None. Which, of course, makes sense: Why would drop_constraint allow you not to specify which constraint you want to drop?
pinkwah
left a comment
pinkwah
left a comment
There was a problem hiding this comment.
Whoops, I meant not to approve
Wip