Skip to content

Comments

refactor to use dry_run, move Tiled jobs into tasks#8

Draft
JunAishima wants to merge 3 commits intoNSLS2:mainfrom
JunAishima:refactor-tasks
Draft

refactor to use dry_run, move Tiled jobs into tasks#8
JunAishima wants to merge 3 commits intoNSLS2:mainfrom
JunAishima:refactor-tasks

Conversation

@JunAishima
Copy link
Contributor

Main objective - enable testing the workflow by adding a dry run option. This will enable the general logic to be tested. In the case of the data validation flow, make the dry run avoid reading the run metadata, and reading the data from the streams.

Secondary objective - reading the data from Tiled is subject to network and server issues. Thus, I have moved these kinds of operations into tasks, with retry capability set.

@JunAishima JunAishima requested a review from AbbyGi February 20, 2026 19:36
Copy link
Contributor

@AbbyGi AbbyGi left a comment

Choose a reason for hiding this comment

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

This is a good idea, just had one minor fix and another comment.

def log_completion(dry_run=False):
logger = get_run_logger()
logger.info("Complete")
logger.info(f"Complete! dry_run:{dry_run}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.info(f"Complete! dry_run:{dry_run}")
logger.info(f"Complete! dry_run: {dry_run}")

logger.info(f"{elapsed_time = }")


@flow
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit weird to call a flow (read_all_streams) from within a flow (data_validation) when everything is related to doing data validation. Maybe we can consider moving whatever is in read_all_streams into data_validation so we only have one flow?

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.

2 participants