Skip to content

Multiple parameter value parsing breaks some plugins #286

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.

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

Closed
felio92 opened this issue Apr 9, 2025 · 7 comments
Closed

Multiple parameter value parsing breaks some plugins #286

felio92 opened this issue Apr 9, 2025 · 7 comments
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested

Comments

@felio92
Copy link

felio92 commented Apr 9, 2025

I've noticed that since the Coming Decade system version was updated from 2502.1.0 to 2504.0.0, parameters are slightly parsed differently. See for example 4066 and 4084, where the former completed successfully before the update but failed in the latter case (which is a re-run of the former). This is because previously it looks like parameters with multiple values such as for SolrField, were parsed as comma-delmited strings, and now they are parsed as lists. I'm a bit confused now, because it looks like the base ParameterType parsed multiple values (as indicated by a comma-delimited string in the default case) as a list for a while now (and not just since the latest release).

The issue is that some of our plugins such as ProblEMS and leadtimeselect expect the plugin config to contain strings for certain parameters, which are then parsed by the plugin. For example the ensemble1 parameter (a SolrField) was previously parsed in 4066 as
s1960-r10i1p1f1,s1960-r1i1p1f1,s1960-r3i1p1f1,s1960-r4i1p1f1,s1960-r6i1p1f1,s1960-r7i1p1f1,
wheras for 4084 it was parsed as:
['s1960-r10i1p1f1', 's1960-r1i1p1f1', 's1960-r3i1p1f1', 's1960-r4i1p1f1', 's1960-r6i1p1f1', 's1960-r7i1p1f1']

I've created a hotfix for the plugins in the meantime, but I still wanted to raise the issue, in case it appears again for other plugins.

Cheers!

@felio92 felio92 added bug Something isn't working help wanted Extra attention is needed question Further information is requested labels Apr 9, 2025
@eelucio
Copy link
Contributor

eelucio commented Apr 9, 2025

maybe I am wrong but I think that this behaviour makes sense, you will need to run (most likely) freva.databrwoser that in case of multiple values for a key parameter will expect lists as entries

@felio92
Copy link
Author

felio92 commented Apr 10, 2025

Yes, the change makes sense. But my point was more that it's a (potentially) breaking change, as many plugins (e.g. leadtimeslect and problems) expect the config to contain a string for the ensemble parameter. I wanted to raise this, as other plugins might also be affected.

@bijanf
Copy link

bijanf commented Apr 16, 2025

@antarcticrainforest any idea on this?

@bijanf
Copy link

bijanf commented Apr 16, 2025

freva-plugin problems variable=tas project1=cmip6 product1=dcpp institute1=mpi-m model1=icon-esm-hr experiment1=dcppa-hindcast1960 time_frequency1=mon ensemble1=s1960-r10i1p1f1,s1960-r1i1p1f1,s1960-r3i1p1f1,s1960-r4i1p1f1,s1960-r6i1p1f1,s1960-r7i1p1f1 grid_label1=gr is_decadal1=true project2=user-b380217 product2=decs4problems institute2=mpi-m model2=mpi-esm1-2-hr experiment2=dcppa-hindcasts* time_frequency2=mon ensemble2=r1i1p1f1,r2i1p1f1,r3i1p1f1,r4i1p1f1 grid_label2=gn is_decadal2=true observation=era5 observation_ensemble=r1i1p1 obs_complete=90 decadals=1960:1968 leadtimes=1 time_frequency_output=year timemean=false region=-20,45,20,70 grid=r720x360 output_dimension=map metrics=rpss mean_or_median=mean time_mean_or_median=mean threshold=perc_1/3,2/3 quantile=0.95 statistic_method=avg anomaly=false significance_method=Bootstrap bootstraps=1000 blocklength=5 ntask=1 outputdir=/work/kd1418/codes/work/k202196/codesdb/output/problems/20240806_113747 cachedir=/work/kd1418/codes/work/k202196/codesdb/cache/problems cacheclear=true debug=false

@eelucio
Copy link
Contributor

eelucio commented Apr 16, 2025

@bijanf , @felix probably solved locally but in this case

problems is failing in at least here:

        if config_dict["ensemble1"] is not None:
            config_dict["ensemble1"] = config_dict["ensemble1"].lower()

        if config_dict["ensemble2"] is not None:
            config_dict["ensemble2"] = config_dict["ensemble2"].lower()

it should be, instead:

if config_dict["ensemble1"] is not None:
    config_dict["ensemble1"] = [item.lower() for item in config_dict["ensemble1"]]

if config_dict["ensemble2"] is not None:
    config_dict["ensemble2"] = [item.lower() for item in config_dict["ensemble2"]]

then it fails in leadtimeselector here:

    if config_dict["ensemble"] != "*":
        ensembles = config_dict["ensemble"].split(",")
    else:
        ensembles = config_dict["ensemble"]

it should be always (I think)

   ensembles = config_dict["ensemble"]

and then it trickles down the error somewhere else, but I lost it, here my run 4111

@antarcticrainforest
Copy link
Member

antarcticrainforest commented Apr 17, 2025

@bijanf that wasn't really a minimum example but anyhow.

If you look at this changes you can see what is going on: src/evaluation_system/api/parameters.py.

The default parameter for max_items used to be 1. Which is kind of weird when you set the multiple=True. But I might not understand the purpose of this multiple flag.

Anyway you can get the desired bahaviour by switching back the old code logic, that is setting max_items=1

import freva
freva.list_plugins()
from problems import ProblEMS
plugin = ProblEMS()
for param in plugin.__parameters__.parameters():
   if param.name == "ensemble1":
      break
param.max_items
# -> 9223372036854775807
plugin.__parameters__.parse_arguments([....,] use_defaults=True)["ensemble1"]
# -> ['s1960-r10i1p1f1',
 's1960-r1i1p1f1',
 's1960-r3i1p1f1',
 's1960-r4i1p1f1',
 's1960-r6i1p1f1',
 's1960-r7i1p1f1']

param.max_items = 1
plugin.__parameters__.parse_arguments(args, use_defaults=True)["ensemble1"]
's1960-r10i1p1f1,s1960-r1i1p1f1,s1960-r3i1p1f1,s1960-r4i1p1f1,s1960-r6i1p1f1,s1960-r7i1p1f1'

So in other words simply put the max_items in your config to 1 and you have the old behaviour!

@bijanf
Copy link

bijanf commented Apr 17, 2025

Thanks @antarcticrainforest !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants