search

payu-org / payu

A workflow management tool for numerical models on the NCI computing systems
Apache License 2.0
21 stars 27 forks source link

FutureWarning import missing #485

Closed anton-seaice closed 3 months ago

anton-seaice commented 3 months ago

Thanks to @hrdawson for spotting this.

When running esm1.5 using payu 1.1.4, payu setup fails at this line:

https://github.com/payu-org/payu/blob/83001a2b45ef786e178894f77dd9fbc0878e73c1/payu/models/um.py#L119

This is because FutureWarning is not imported.

Traceback (most recent call last):
  File "/g/data/vk83/apps/payu/1.1.4/bin/payu", line 10, in <module>
    sys.exit(parse())
  File "/g/data/vk83/apps/payu/1.1.4/lib/python3.10/site-packages/payu/cli.py", line 41, in parse
    run_cmd(**args)
  File "/g/data/vk83/apps/payu/1.1.4/lib/python3.10/site-packages/payu/subcommands/setup_cmd.py", line 26, in runcmd
    expt.setup(force_archive=force_archive)
  File "/g/data/vk83/apps/payu/1.1.4/lib/python3.10/site-packages/payu/experiment.py", line 439, in setup
    model.setup()
  File "/g/data/vk83/apps/payu/1.1.4/lib/python3.10/site-packages/payu/models/um.py", line 119, in setup
    raise FutureWarning(
FutureWarning: The `um_env.py` configuration file has been deprecated and should be relplaced with a yaml file. Convert `um_env.py` to `um_env.yaml` using [https://github.com/ACCESS-NRI/esm1.5-scripts/blob/main/config-files/UM/um_env_to_yaml.py]

Also the text is slightly unclear ... I think we expect um_env.py configuration to still work, so the warning text should be in the future tense (will be deprecated)

jo-basevi commented 3 months ago

Thanks for creating this issue! Yeah it looks like it is treating FutureWarning as an exception.

@aidanheerdegen @blimlim, do we want to support loading environment variables from both um_env.py and um_env.yaml and so change the exception to a warning? If so, I have some changes in this branch that I think will implement that though will need to test it before opening a PR.

blimlim commented 3 months ago

From what I remember at the time when discussing with @aidanheerdegen, it sounded like the best option was to deprecate the old um_env.py file rather than keeping support for it due to security issues with sourcing a python file, and so the desired behaviour was for um_env.py to no longer work with the latest release, and for an exception rather than warning to be raised. @aidanheerdegen just wanted to check whether this sounds right to you.

I'm not too good with all the different exception/warning types, but it seemed like raising a FutureWarning as an exception was the best option for raising an error about an already deprecated feature, though definitely agree that any ways we can make it a bit clearer would be great!

anton-seaice commented 3 months ago

Oh I see! If its trying to be an exception then we should raise an Error rather than a warning !

jo-basevi commented 3 months ago

Ok, would a RuntimeError make sense? I am not sure of the best standard error types for deprecated features either

blimlim commented 3 months ago

Oh I see! If its trying to be an exception then we should raise an Error rather than a warning !

Good idea!

Ok, would a RuntimeError make sense?

I think that sounds ok!

I just noticed a typo in the error message I'd written - should be "replaced" not "relplaced" :( .... If we are fixing this up anyway, do you think there are any changes that could make it a bit clearer? I'm wondering whether "... configuration file is no longer supported" is clearer than "... has been deprecated", or do you think it's much the same?

jo-basevi commented 3 months ago

Yeah I think "no longer supported" makes more sense, as deprecated usually means it will be eventually removed in future releases but can still be used.