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

Sbachmei/mic 5476/add optional schema arg to config constructor #127

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

stevebachmeier
Copy link
Collaborator

Add optional schema arg to Config constructor

Description

Changes and notes

This implements a new and optional arg to the Config constructor
that allows us to pass in an on-the-fly allowable PipelineSchema.
This is primarily useful for testing purposes.

This unlocked for two new tests along the way.

Verification and Testing

slow tests pass

subgraph = ImplementationGraph(self).subgraph(nodes)
# NOTE: The subgraph is not necessarily able to be topologically sorted
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@patricktnast Does this note jive with your understanding? We should talk about this. I'm confused why an nx graph isn't just always sorted. And, related, I'm confused why I am unable to access the is_sorted attr sometimes (it's not False, it's actually not set)

from easylink.step import HierarchicalStep, InputStep, LoopStep, OutputStep, Step
from easylink.utilities.validation_utils import validate_input_file_dummy

SINGLE_STEP_NODES = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SINGLE_STEP_NODES, _EDGES, and _SCHEMA_PARAMS were oved from old pipeline_schema_constants/integration_test.py (the file deleted above). They are identical except for the names.

SINGLE_STEP_SCHEMA_PARAMS = (SINGLE_STEP_NODES, SINGLE_STEP_EDGES)


BAD_COMBINED_TOPOLOGY_NODES = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

everything below here is new

@@ -575,40 +578,53 @@ def test_merge_combined_implementations_parallel(default_config_params, test_dir
check_nodes_and_edges(pipeline_graph, expected_nodes, expected_edges)


def test_cycle_error(default_config_params) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These three tests are identical and the two new ones are as well aside from needing to pass in a custom PipelineSchema - hence the refactor to a single parametrized test.

Copy link
Collaborator

@zmbc zmbc left a comment

Choose a reason for hiding this comment

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

Overall looks good. If we do a lot more of this I think it is going to get weird that the implementations are all mixed together in implementation_metadata.yaml, but I think it is okay with just these.

env:
INPUT_ENV_VARS: "DUMMY_CONTAINER_MAIN_INPUT_FILE_PATHS,DUMMY_CONTAINER_SECONDARY_INPUT_FILE_PATHS"
outputs:
step_4b_main_output: result.parquet
step_4b_r:
steps:
- step_4b
image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/python_pandas.sif
script_cmd: python /dummy_step.py
image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/r-image.sif
Copy link
Collaborator

Choose a reason for hiding this comment

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

oops, good catch

@stevebachmeier
Copy link
Collaborator Author

Overall looks good. If we do a lot more of this I think it is going to get weird that the implementations are all mixed together in implementation_metadata.yaml, but I think it is okay with just these.

Yeah I agree. I'll think about creating different metadata files at some point. But I'm not even sure this will end up being the actual container for "real" metadata anyway, right?

@stevebachmeier stevebachmeier requested a review from zmbc December 16, 2024 19:44
@stevebachmeier stevebachmeier merged commit 27555c9 into main Dec 16, 2024
6 of 9 checks passed
@stevebachmeier stevebachmeier deleted the sbachmei/mic-5476/add-schema-arg-to-config branch December 16, 2024 19:52
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.

4 participants