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.

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

Various #374

Merged
merged 6 commits into from
Mar 6, 2025
Merged

Various #374

merged 6 commits into from
Mar 6, 2025

Conversation

ceblanton
Copy link
Collaborator

@ceblanton ceblanton commented Mar 1, 2025 •
edited
Loading

Describe your changes

  1. Update schema submodule to constrain analysis configs
  2. Some massaging of start/stop times specified as years. (cylc interprets "0001" as a year but not "1")
  3. Logging

Issue ticket number and link (if applicable)

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback
  • No print statements; all user-facing info uses logging module

Chris Blanton added 4 commits February 28, 2025 21:48
Fix path to combined yaml printed to stdout
45ada2a
fre pp configure-yaml updates
7d2fe48 
- use logging instead of prints
- catch schema parsing errors and report as internal errors
- convert integers to str during rose output writing
Workaround to specify pp start/stop as YYYY if specified in yaml as a…
a598602 
…n integer.

If specified in ISO dates, then leave it.
Update schema to 2025.03
f4e1ecc
@ceblanton ceblanton changed the title Analysis related Various Mar 5, 2025
@ceblanton ceblanton marked this pull request as ready for review March 5, 2025 19:32
@ceblanton
Copy link
Collaborator Author

ceblanton commented Mar 5, 2025 •
edited by ilaflott
Loading

@ilaflott same story on this one. changes we definitely want but not the final update (surprise surprise)

ilaflott
ilaflott requested changes Mar 5, 2025
View reviewed changes
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

some feedback, all nits.

biggest problem is the failing pipeline.

fre/pp/configure_script_yaml.py
import yaml
import metomi.rose.config

import fre.yamltools.combine_yamls_script as cy

import logging
logging.basicConfig()
Copy link
Member

Choose a reason for hiding this comment

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

in fre-cli the configuration for logger gets set in fre.py- where it can pick up a verbose flag... e.g. with fre -v yamltools configure-yaml ARGS, so this doesn't need to be done here, unless you really want the secondary config on top of the base fre call

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha! So there is a central logger, and we're unnecessary re-declaring it in each python file now?

Copy link
Member

@ilaflott ilaflott Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

no you do gotta declare it in each file. that's the suggested basic logging usage. the logging how-to isn't so bad a read. there's fancier things we could be doing, i'm sure.

the configuration of logging is centralized though, so no need to do basicConfig unless you want something tool-specific.

@Ciheim and I were discussing global v. tool-specific verbose the other day, maybe we should have it again!

Copy link
Member

@ilaflott ilaflott Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

for the global-configuring, see fre/fre.py#L15-L18 and fre/fre.py#L67-L76

@ceblanton
Copy link
Collaborator Author

The tests are failing from some pp yaml validation and usage tests which broke from the schema changes. It's a good catch. I'll update the tests with the schema changes and then they should pass.

ilaflott
ilaflott approved these changes Mar 6, 2025
View reviewed changes
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

thank you for the yaml updates!

@ilaflott ilaflott merged commit d1184c2 into main Mar 6, 2025
2 checks passed
@ilaflott ilaflott deleted the analysis-related branch March 6, 2025 00:06
@ceblanton
Copy link
Collaborator Author

I was going to try to see if I could get the central logging going in a few minutes, which would be a good idea normally as it sets an example, but thank you for merging. The tests were excellent and easy to adjust.

You'll teach me the proper central logging way and I'll help fix as I go like you do.

@ilaflott
Copy link
Member

ilaflott commented Mar 6, 2025

I was going to try to see if I could get the central logging going in a few minutes, which would be a good idea normally as it sets an example, but thank you for merging. The tests were excellent and easy to adjust.

You'll teach me the proper central logging way and I'll help fix as I go like you do.

sorry for merging too quick! I invite you to rebase and open another PR for your experiment in central logging though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilaflott ilaflott ilaflott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ceblanton @ilaflott