Remove print statements for logging (#421) #439
Conversation
|
@enssow : can you make sure the code is properly ruffed. |
There was a problem hiding this comment.
Looks good, but it changes the way loss is outputed in slurm mode. Instead of output file it's now printed to error file. Is this intentional?
Personally I'm not a fan because it's makes it impossible to quickly check the training by running tail output_{run_id}.txt
|
@kacpnowak Is that in the |
Yes, we should keep the infos separate from warning, errors etc. There is also a PR to have a proper redirection of the different streams to different files to make sure things are visible: #271 . This would be the ultimate solution. To which stream does __logger.info go? |
There was a problem hiding this comment.
Thank you Sorcha for the change. Regarding the failing integration test: indeed, a fix had been reverted, let's do it separately.
Have you been able make a limited run with real data? I am a bit surprised by the comment of @kacpnowak since before this PR, the print and logging statement are both redirected to the output_xxx.txt:
0: 023 : 01020/01024 : 024572 : loss = 2.9625E-02 (lr=9.77E-10, s/sec=0.746)
0: ERA5 : 2.9625E-02
0:
1: 2025-07-05 05:01:09,709 183795 trainer.py:321 : INFO : Epoch 23 of 24: validate.
3: 2025-07-05 05:01:09,709 183797 trainer.py:321 : INFO : Epoch 23 of 24: validate.
| if __name__ == "__main__": | ||
| print(get_run_id()) |
There was a problem hiding this comment.
actually we can fully remove this main block. This is meant for people who want to create their own run ids.This is also implemented in the private repo, and it has no dependencies.
There was a problem hiding this comment.
I can't seem to find any dependencies on this file so can I go ahead and delete the whole file?
Due to using default value in StreamHandler it always goes to stderr |
It's really weird. Maybe it's due to different batch scripts? In my case I have set two different files for those streams: I pase below part of my err file |
|
That's how the default SLURM looks like Maybe the mixing with the sbatch and piping explains the behavior |
|
Following yesterday's meeting I've reverted this to a draft as there are still some questions I have to sort out,
So I had been using the |
The content of the log and output folders comes from the above lines in a batch slurm script. In an interactive session everything is just written to the terminal. |
|
I've done the ruff fixes and removed |
scripts/check_gh_issue.py
Outdated
| @@ -1,3 +1,5 @@ | |||
| # ruff: noqa: T201 | |||
Description
Type of Change
Issue Number
Closes #421
Removes print statements from files except for
src/weathergen/model/model.pyand edits other print statements to be loggedCode Compatibility
Code Performance and Testing
uv run trainand (if necessary)uv run evaluateon a least one GPU node and it works$WEATHER_GENERATOR_PRIVATEdirectoryDependencies
Documentation
Additional Notes
uv run pytest ./integration_tests/small1_test.py --verbosewhich returnedE ImportError: cannot import name 'inference_from_args' from 'weathergen' (unknown location)