IAMconsortium / units

Common unit definitions for integrated assessment research
GNU General Public License v3.0
20 stars 11 forks source link

"Unknown conversion specified" in test suite with pint 0.18 #31

Closed phackstock closed 2 years ago

phackstock commented 3 years ago

A new version of pint (0.18) was released last week (2021.10.26), which caused the pyam tests connected to units to fail. An easy and quick fix would be to pin the version of pint <= 0.17. Additionally, it might be a good idea to set up nightly tests that run at least once a week so that we catch these dependency changes.

khaeru commented 3 years ago

Can we add some information on how the tests fail, and what exactly was the offending change in Pint (e.g. a link to the changelog)?

Introducing pins in setup.cfg is not totally verboten, but shouldn't be done without first considering how difficult it would be to immediately follow changes upstream. If the answer is "very difficult," then we can consider it.

khaeru commented 2 years ago

Can we add some information on how the tests fail, and what exactly was the offending change in Pint (e.g. a link to the changelog)?

Since this is blocking #34, just opened, I will do it.

khaeru commented 2 years ago

The error messages (e.g. here) look like:

iam_units/__init__.py:136: in format_mass
    return format_unit(to_units_container(dict(units), registry=registry), spec)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

unit = <UnitsContainer({'kilogram CO2': 1, 'year': -1})>, spec = ':C'
registry = None, options = {}, fmt = None

    def format_unit(unit, spec, registry=None, **options):
        # registry may be None to allow formatting `UnitsContainer` objects
        # in that case, the spec may not be "Lx"

        if not unit:
            if spec.endswith("%"):
                return ""
            else:
                return "dimensionless"

        if not spec:
            spec = "D"

        fmt = _FORMATTERS.get(spec)
        if fmt is None:
>           raise ValueError(f"Unknown conversion specified: {spec}")
E           ValueError: Unknown conversion specified: :C

/opt/hostedtoolcache/Python/3.10.0/x64/lib/python3.10/site-packages/pint/formatting.py:408: ValueError

I also see in places:

E           ValueError: Unknown conversion specified: :~
khaeru commented 2 years ago

The changelog for pint 0.18 is here. Nothing jumps out. But someone opened hgrecco/pint#1407 2 days after this issue, so it appears to be an unanticipated breakage and possibly headed towards resolution.

khaeru commented 2 years ago

@phackstock if you also want to monitor for pint 0.18.1 and, once it's released, make another PR to revert #32, then we can approve and merge that PR. Otherwise, I'd prefer to wait 🙂

danielhuppmann commented 2 years ago

I'm not sure I interpret the discussion in the pint-repo (https://github.com/hgrecco/pint/issues/1407) that this was an unanticipated change to be resolved soon - sounds to me like they'll just reinsert and properly deprecate the breaking change.

So I suggest that we add the pin, and @phackstock puts it on his to-do list for next week to investigate how this can be properly fixed.

phackstock commented 2 years ago

I subscribed to the issue and set 'watch' for new releases so I should be notified if anything changes there. But the way I read it, is that it is intended behavior that they just didn't document well. The strategy they were discussing is rolling it back for 18.1 but including it for good in 19.0. Still that would move the problem for us.

khaeru commented 2 years ago

If the change is permanent, then we should adjust to it. I went ahead and looked, and it turns out that's simple. See #35.