Skip to content

Issue #689: Fix prepare generate logging for substreams #703

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.

Sign up for GitHub

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

Jump to bottom
Merged
mo-gill merged 6 commits into from
Nov 25, 2025
Merged

Issue #689: Fix prepare generate logging for substreams #703

mo-gill merged 6 commits into from
Nov 25, 2025

Conversation

@mo-gill
Copy link
Collaborator

@mo-gill mo-gill commented Nov 21, 2025

Closes issue #689

@mo-gill mo-gill self-assigned this Nov 21, 2025
@mo-gill mo-gill added the enhancement New feature or request label Nov 21, 2025
@mo-gill mo-gill added this to the CDDS v3.4.0 milestone Nov 21, 2025
@mo-gill mo-gill marked this pull request as ready for review November 21, 2025 14:11
mo-jareddrayton
mo-jareddrayton requested changes Nov 24, 2025
View reviewed changes
Copy link
Collaborator

@mo-jareddrayton mo-jareddrayton left a comment

Choose a reason for hiding this comment

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

Looks good, only one minor suggestion. All tests are passing.

(It would be nice to solve the repeated output, but I assume it will take some messing around with the logger.)

cdds/cdds/tests/test_prepare/test_generate.py
Copy link
Collaborator

@mo-jareddrayton mo-jareddrayton Nov 24, 2025

Choose a reason for hiding this comment

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

I think the tests would be more legible if you moved the self.var_list* variables into their respective unit tests.

Copy link
Collaborator Author

@mo-gill mo-gill Nov 25, 2025

Choose a reason for hiding this comment

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

Have adjusted this here: b2c9bd4

@mo-gill
Copy link
Collaborator Author

mo-gill commented Nov 25, 2025

Looks good, only one minor suggestion. All tests are passing.

(It would be nice to solve the repeated output, but I assume it will take some messing around with the logger.)

Have adjusted the logging behaviour here:3bf5c7a

I've tested it and it seems to fix the duplication whilst keeping the log showing via cylc logs

mo-jareddrayton
mo-jareddrayton approved these changes Nov 25, 2025
View reviewed changes
Copy link
Collaborator

@mo-jareddrayton mo-jareddrayton left a comment

Choose a reason for hiding this comment

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

Looks good. The removal of the duplicate logging is a nice QoL improvement.

Tests are passing.

@mo-gill mo-gill merged commit 6893cea into main Nov 25, 2025
1 check passed
@mo-jareddrayton mo-jareddrayton mentioned this pull request Dec 3, 2025
Closed
@matthew-mizielinski matthew-mizielinski deleted the 689_prep_generate_logging_substreams branch December 15, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mo-jareddrayton mo-jareddrayton mo-jareddrayton approved these changes

@matthew-mizielinski matthew-mizielinski Awaiting requested review from matthew-mizielinski

@mo-laurenboon mo-laurenboon Awaiting requested review from mo-laurenboon

Assignees

@mo-gill mo-gill

Labels

enhancement New feature or request

Projects

None yet

Milestone

CDDS v3.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mo-gill @mo-jareddrayton