Add generic task_config and task_activity#542
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
app/db/model.py
Outdated
| parent_id: Mapped[uuid.UUID | None] = mapped_column( | ||
| ForeignKey(f"{EntityType.task_config}.id"), index=True | ||
| ) |
There was a problem hiding this comment.
parent_id seems a bit misleading as it implies a hierarchy. Could we use a name like activity_id/generator_activity_id or something along these lines.?
There was a problem hiding this comment.
It should be clear that parent_id isn't the activity, but it's an optional task_config that was used by the activity to generate this task_config.
In theory it's not strictly required, because it's possible to get it from task_activity.used[0], but it can be handy for filtering or to retrieve the full parent in a simpler way, and it would ensure that it's unique.
For the existing campaigns, it would be the equivalent of:
- Simulation.simulation_campaign_id
- SkeletonizationConfig.skeletonization_campaign_id
- IonChannelModelingConfig.ion_channel_modeling_campaign_id
Any other suggestion about the name if we keep it? task_config_generator_id?
There was a problem hiding this comment.
Note that the attribute in opposite direction (parent task -> children tasks) isn't defined, since it's not ensured that all the children belongs to the same activity (unless we allow only one activity per config)
There was a problem hiding this comment.
I am good with task_config_generator_id
app/db/model.py
Outdated
| parent_id: Mapped[uuid.UUID | None] = mapped_column( | ||
| ForeignKey(f"{EntityType.task_config}.id"), index=True | ||
| ) | ||
| input: Mapped[list[Entity]] = relationship( |
There was a problem hiding this comment.
Given that this is a list, I would go with "inputs"
There was a problem hiding this comment.
Agreed that input (singular) is misleading, although the attribute is called input_ids so it would match the prefix.
I'll rename it to inputs unless we have a better name.
|
There are some conceptual differences between how we planned to use Campaign and TaskConfig in the other PR which are worth mentioning: A) Campaign->scan_parameters is intended to hold a dictionary where the keys are the names of the variables to be scanned over (i.e. stimulus_strength), and the values are lists of values to be scanned over in that dimension (i.e. [0.1, 0.2, 0.3]). Perhaps it's possible that a second or third task in a workflow might also scan over several values though, so maybe it's not just the top Campaign config that needs to hold a scan_parameters with lists. Maybe a generic TaskConfig object could have at least one of the two optional attributes:
We might have a third variable for convenience which is essentially B, called something like coordinate_parameters Let me know if I can explain that better Tagging @chr-pok who might also have some thoughts on this |
This is important to discuss, but from the point of view of entitycore, the JSON dicts are currently arbitrary json blobs that can contain any data, that is interpreted only by the client (obi-one). We could also rename the attribute to something more generic instead of To keep the model simpler, it would be better to avoid adding more jsonb fields, and consolidate these details in a single json that can depend on the task_config_type. For reference, these might be related to the point C above: #462 and the corresponding PR #441 |
+1 For this. There is no need to have scan_parameters + scan_index. We can aim for one field with the desired schema. |
Thanks for this PR! Since campaigns and individual configs are conceptually different, I am not quite sure if this level of generalization is really needed. I personally would prefer to keep them (as well as the corresponding activities) separate, as in the alternative PR#532. This would allow us to be more flexible w.r.t. specific attributes required for campaigns vs. single configs (which will probably be displayed/filtered differently in the UI, etc.). Or is there a compelling reason why these even more generic models would be beneficial? |
From the point of view of entitycore, the logic to handle campaigns and individual configs is the same, with the only difference that the campaign is the first step of the workflow, while configs are the second step.
With the approach in this PR, it means that they are stored in entitycore in the same table, and can be created or retrieved using the same endpoint, with just different parameters.
Do you have in mind specific attributes that need to be specified for the campaign and not for single configs (or the other way around), that cannot be part of the json configuration?
It's not a compelling reason, but the generalization:
Note that the json config called {
"version": 1,
"type": "circuit_simulation__campaign",
"scan_parameters": {}
}{
"version": 1,
"type": "circuit_simulation__config",
"scan_parameters": {},
"scan_index": {}
}entitycore doesn't enforce any schema at the moment, but it could be done if needed even with the generic approach, as long as there is a type that can be used as a discriminator. |
|
The only thing which feels strange to me is that there is no way to programmatically distinguish whether TaskConfigs or TaskActvities of a specific task_config_type are intended as a single coordinate or "scan", when we are indeed deciding this when we make a new task_config_type (i.e. circuit_simulation__campaign). Looking at the contents of scan_parameters to determine this, is putting the burden on the client (i.e. obi-one), when I think we already know this when we define new task_config_types Maybe this could come in useful, for example when visualising (or recursing through) a graph of a complex multi-stage workflow (i.e. a long chain of configs and tasks). Maybe just a simple representation of which task_config_types are intended as a scan configs or coordinates. I'm not sure what form this would take Another thing to consider is campaign vs single coordinate analysis. Eventually we might want to link an analysis result to a campaign or coordinate. I'm not sure how best to do that, but another thing to bear in mind. Maybe it's ok for this to be generic between campaigns and coordinates. Or maybe a campaign analysis result should be linked directly to the coordinates or generated entities (or a subset of them when the analysis is of a subset of coordinates or entities in the "campaign". It was a common pattern at bbp to only analyse a subset of simulations for example |
I don't follow what is needed to be distinguished, and how this PR is different from the previous one in this regard.
I think we can take the discussion offline for more clarifications. |
|
Say we want a generic tool for visualising (or operating on) the graph of any workflow, and we want to visualise (or operate on) the campaign configs differently from the single configs. We would just need a list somewhere of which task_config_types are "campaigns" or "single" configs. That could go in entitycore somewhere perhaps, rather than detecting whether __campaign is in the string Can discuss further offline |
5 participants
This PR adds more generic models compared to #532.
Only TaskConfig and TaskActivity are defined, and can replace the previous models according to this mapping:
where:
task_config_type (enum):
...
task_activity_type (enum):
...
This would allow to define workflows of task_config with more than 2 activities.
This generalization is possible only if:
- all the task_configs (campaign and config) share the same attributes (scan_parameters, parent_id, input)
- all the task_activities (config_generation and execution) share the same attributes (executor, execution_id)