Drop pint higher-bound

[LecrisUT]

Pull Request Checklist

• implement the feature

Closes #2740

[LecrisUT]

In principle this should be handled by /plans/install/pip right? I don't see a pip install . there to install dependencies though :/
(Oh I see: /tmp/venv/bin/pip install .[all], not ideal though)

My other concern with that approach is that it is gated by rpm build, which I wanted to avoid for debugging and using Pint >= 0.20 for testing.

Other approaches could be:

• Run tmt from Github Action
  • Pro: Fine-control of python version, dependencies, etc.
  • Con: Hard to get legible results/logs/artifacts
• Run testing-farm from Github Action or add a packit configuration with skip_build
  • Pro: Good reports
  • Con: Less control over the python version, dependencies, etc.

[martinhoyer]

Hi @LecrisUT, what is the goal here?

[LecrisUT]

Hi @LecrisUT, what is the goal here?

To close #2740, and address https://src.fedoraproject.org/rpms/python-pint/pull-request/10

Stuck at how to do the testing though.

[happz]

Hi @LecrisUT, what is the goal here?

To close #2740, and address https://src.fedoraproject.org/rpms/python-pint/pull-request/10

Stuck at how to do the testing though.

Did you run unit tests with newer Pint installed? I had bad experience because of chanes Pint made, failing imports and changed internals.

Would you mind blocking this on pyright coverage of tmt.hardware? I have it in my git stash, I’ll make a PR out of it after lunch. I suppose it'd be safer to mess with versions of libraries after that.

[LecrisUT]

Would you mind blocking this on pyright coverage of tmt.hardware?

Not sure what you mean by blocking here. I have for now disabled the generic tpye-checking on pint which did not seem to work anymore ¹².

Did you run unit tests with newer Pint installed? I had bad experience because of chanes Pint made, failing imports and changed internals.

Not yet, I'll try to run them now. Didn't cross my mind that hatch env will work to test locally.

🤔 Couldn't find any plans that I could run that includes /tests/provision/hardware

$ tmt -c trigger=commit -vv run -a provision -h local test -n /tests/provision/hardware

At least python3 -c "import tmt.hardware" does not fail

Footnotes

1. https://github.com/hgrecco/pint/issues/1166 ↩

2. https://github.com/hgrecco/pint/pull/1259 ↩

[LecrisUT]

Tried to do locally the tests:

$ cd tests/provision/hardware/data
$ tmt plans show /plan/local
    warn: /plan/local:provision - {'hardware': {'boot': {'method': 'bios'}, 'compatible': {'distro': ['rhel-7', 'rhel-8']}, 'cpu': {'sockets': '<= 1', 'cores': 2, 'threads': '>= 8', 'cores-per-socket': '= 2', 'threads-per-core': '== 4', 'processors': '> 8', 'model': 62, 'model-name': '!~ Haswell', 'family': '< 6', 'family-name': 'Skylake'}, 'disk': [{'size': '40 GiB'}, {'size': '>= 120 GiB'}], 'hostname': '~ .*.foo.redhat.com', 'memory': '8 GiB', 'network': [{'type': 'eth'}, {'type': 'eth'}], 'tpm': {'version': '2.0'}, 'virtualization': {'is-supported': True, 'is-virtualized': False, 'hypervisor': '~ xen'}}, 'how': 'local'} is not valid under any of the given schemas
/plan/local

But the same issue is when I use either pint versions

Edit: nvm, it works on /plan/artemis

$ tmt -vv plans show /plan/artemis
/plan/artemis
                discover 
               provision 
                     how artemis
             api-version 0.0.69
                    user root
                  become false
       provision-timeout 600
          priority-group default-priority
                 keyname default
                     key 
                 api-url http://127.0.0.1:8001
               user-data {}
               kickstart {}
          provision-tick 60
             api-retries 10
              ssh-option 
             api-timeout 10
 skip-prepare-verify-ssh false
api-retry-backoff-factor 1
                log-type 
                    arch x86_64
                hardware boot:
                             method: bios
                         compatible:
                             distro:
                               - rhel-7
                               - rhel-8
                         cpu:
                             sockets: <= 1
                             cores: 2
                             threads: '>= 8'
                             cores-per-socket: = 2
                             threads-per-core: == 4
                             processors: '> 8'
                             model: 62
                             model-name: '!~ Haswell'
                             family: < 6
                             family-name: Skylake
                         disk:
                           - size: 40 GiB
                           - size: '>= 120 GiB'
                         hostname: ~ .*.foo.redhat.com
                         memory: 8 GiB
                         network:
                           - type: eth
                           - type: eth
                         tpm:
                             version: '2.0'
                         virtualization:
                             is-supported: true
                             is-virtualized: false
                             hypervisor: ~ xen
                 prepare 
                     how shell
                  script 
                   where 
                 execute 
                     how tmt
         no-progress-bar false
                  script 
                   where 
              exit-first false
             interactive false
                duration 1h
                  report 
                     how display
           display-guest auto
                  finish 
                     how shell
                  script 
                   where 
                 enabled true
                 sources /home/lecris/PycharmProjects/tmt/tests/provision/hardware/data/main.fmf
                  fmf-id url: https://github.com/LecrisUT/tmt.git
                         ref: fix/2740
                         path: /tests/provision/hardware/data
                         name: /plan/artemis
                     web https://github.com/LecrisUT/tmt/tree/fix/2740/tests/provision/hardware/data/main.fmf

[martinhoyer]

@LecrisUT

To close #2740, and address https://src.fedoraproject.org/rpms/python-pint/pull-request/10

I see, thanks!

Not sure What you mean by blocking

I'm guessing marking this PR as being blocked by pyright tmt.hardware coverage PR.

[happz]

Would you mind blocking this on pyright coverage of tmt.hardware?

Not sure what you mean by blocking here. I have for now disabled the generic tpye-checking on pint which did not seem to work anymore 12.

Yeah, but that's demonstrating the problem right away :) When I drop the inner type, and run mypy with Pint 0.19.2, which is a perfectly valid version, mypy starts reporting the following:

tmt/hardware.py:162: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/hardware.py:163: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/hardware.py:643: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:181: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:182: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:191: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:219: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:264: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:265: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:273: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:274: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:346: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:347: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/testcloud.py:589: error: Missing type parameters for generic type "Size"  [type-arg]
tmt/steps/provision/mrack.py:220: error: Missing type parameters for generic type "tmt.hardware.Size"  [type-arg]
tmt/steps/provision/mrack.py:229: error: Missing type parameters for generic type "tmt.hardware.Size"  [type-arg]

At least in some versions of Pint, Quantity seemed like a generic type to mypy. And that's the problem, it's a weird library in this sense, and I am very much worried that by tailoring our code to match new versions we would get too far from versions shipped in RPMs that tmt actually uses in "production". We might end up with annotations being wrapped in strings, or some conditional type aliases, but I'm afraid the answer won't be as simple as dropping the generic nature of Quantity :(

I don't mind allowing newer Pint, I would just like to be sure tmt.hardware still works with older ones, which means more tests and better coverage. I'd extend pyright coverage to tmt.hardware, there's also an attempt in the package manager PR to run unit tests against system packages (https://github.com/teemtee/tmt/pull/2557/files#diff-cac89b0b4c2adab3aaef0788b4c8179536421b4a4e3d68ec279d2e4e2e31ff24R32). Hence my proposal to not merge this PR before adding more coverage.

Did you run unit tests with newer Pint installed? I had bad experience because of chanes Pint made, failing imports and changed internals.

Not yet, I'll try to run them now. Didn't cross my mind that hatch env will work to test locally.

/tests/unit should cover it, too, e.g.

tmt -c trigger=commit -vv run -a provision -h local plan -n '^/plans/features/core$' test -n '^/tests/unit$'

I tried to play with something else, but when executed from hatch -e dev shell, one can manipulate packages with pip.

[LecrisUT]

Yeah, but that's demonstrating the problem right away :) When I drop the inner type, and run mypy with Pint 0.19.2, which is a perfectly valid version, mypy starts reporting the following:

We can conditionally add the Quantity[int] on the versions before 0.20, wdyt. It would also depend on how upstream wants to fix the typing. Alternatively, we can copy an implementation that had the appropriate typing format (could be tricky since the _typing.py did not actually change since the upstream PR), or alias it to Any until they better support it?

Fortunately, this does not seem to affect the actual code behavior yet.

I don't mind allowing newer Pint, I would just like to be sure tmt.hardware still works with older ones, which means more tests and better coverage.

Oh, now I see, there were the ignore files there 👍

[LecrisUT]

Lol, I like it how PyRight is contradicting itself

  /home/runner/work/tmt/tmt/tmt/hardware.py:544:20 - error: Unnecessary isinstance call; "Quantity" is always an instance of "Quantity" (reportUnnecessaryIsInstance)
  /home/runner/work/tmt/tmt/tmt/hardware.py:545:25 - error: Expression of type "PlainQuantity[Any]" cannot be assigned to declared type "ConstraintValue"

Interesting though, Quantity seems to be an alias of PlainQuantity[Any] which does seem to accept generic type.

[happz]

Yeah, but that's demonstrating the problem right away :) When I drop the inner type, and run mypy with Pint 0.19.2, which is a perfectly valid version, mypy starts reporting the following:

We can conditionally add the Quantity[int] on the versions before 0.20, wdyt.

That would make sense, yes.

It would also depend on how upstream wants to fix the typing. Alternatively, we can copy an implementation that had the appropriate typing format (could be tricky since the _typing.py did not actually change since the upstream PR), or alias it to Any until they better support it?

I'd like to avoid copying implementations if at least possible, therefore I for one would prefer the version-dependant type definitions, conditional imports, TypeAliases, and so on, as needed. Aliasing to Any should be the last resort, I hope we could do better :)

Fortunately, this does not seem to affect the actual code behavior yet.

I don't mind allowing newer Pint, I would just like to be sure tmt.hardware still works with older ones, which means more tests and better coverage.

Oh, now I see, there were the ignore files there 👍

Anyway, flushed by git stash into #2742, that enables pyright. #2557 should add unit tests against system packages, which I consider even stronger check, that should prevent the most of issues from sneaking in - if our code is broken with python-pint from EPEL9, this test should reveal it.

[LecrisUT]

I'll rebase when #2742, and if all goes well, we can delay this for after #2557 to make sure we don't have any surprises.

Seems neither pyright nor mypy can handle:

    if version.parse(importlib.metadata.version("pint")) >= version.parse("0.20"):
        Size: TypeAlias = Quantity
    else:
        Size: TypeAlias = 'Quantity[int]'

[happz]

I'll rebase when #2742, and if all goes well, we can delay this for after #2557 to make sure we don't have any surprises.

Seems neither pyright nor mypy can handle:

    if version.parse(importlib.metadata.version("pint")) >= version.parse("0.20"):
        Size: TypeAlias = Quantity
    else:
        Size: TypeAlias = 'Quantity[int]'

Hm, type aliases cannot be declared conditionally. Type checkers do have some basic support for testing Python versions, but a test like this is way too runtime-ish for their taste :/

This sucks, and I would hate to use Any :( We still have to live with older Pint versions, and newer ones seem to be too incompatible on this level... Any ideas?

[LecrisUT]

This sucks, and I would hate to use Any :( We still have to live with older Pint versions, and newer ones seem to be too incompatible on this level... Any ideas?

To do this cleanly, no it's rather hard. At the same time, how much do we gain by doing type-checking with different versions of pint? For this one, we want to make sure it is a pint-like Quantity, and although we lose the int checker, that could be added at runtime.

These are mostly development checks to make sure we don't shoot ourselves in the foot, so I think it's ok for now to relax these constraints to Quantity or maybe PlainQuantity[int] until upstream can better support the typing.

[happz]

This sucks, and I would hate to use Any :( We still have to live with older Pint versions, and newer ones seem to be too incompatible on this level... Any ideas?

To do this cleanly, no it's rather hard. At the same time, how much do we gain by doing type-checking with different versions of pint?

A lot, I would say, because mypy and pyright are not just about type checking, they also detect attributes that are not defined, for example. Most tmt users depend on tmt that's packaged and shipped in RPMs. Relaxing bounds on dependencies can easily lead to us, developers, using new functionality that's not available to end users. Pint 0.24 may add the PlainQuantity.magnitude_but_better attribute, we start using it in code, and if we don't have the relevant code covered by functional tests, nothing will stop us from shipping it to CentOS Stream 9 where only Pint 0.16 is available.

For this one, we want to make sure it is a pint-like Quantity, and although we lose the int checker, that could be added at runtime.

These are mostly development checks to make sure we don't shoot ourselves in the foot, so I think it's ok for now to relax these constraints to Quantity or maybe PlainQuantity[int] until upstream can better support the typing.

Yeah, but that's just one possible way how we could harm ourselves. PlainQuantity may offer new methods and new functionality, do we have a test that would prevent us from shipping it to CentOS Stream 9 without noticing? A return type may change between versions, what's safe with Pint 0.23 may be unsafe with Pint 0.16, e.g. an explicit str() call might be needed to turn int to str we were used to.

This starts to suddenly feel more and more chilling and dangerous :) Frankly, I don't understand why we don't have upper bounds on other requirements, or why don't we run mypy/pyright against system packages as a dedicated test in /plans/features/core :O

I mean, thinking about what are we trying here, I'd say we really should extend the coverage into this area first, and I have no idea how could I missed this hole, why don't we have something like this already - yeah, Pint 0.99 in pre-commit is nice and cool, but it's no way near to what tmt will face in CentOS... This can escalate quite easily.

[LecrisUT]

A lot, I would say, because mypy and pyright are not just about type checking, they also detect attributes that are not defined, for example. Most tmt users depend on tmt that's packaged and shipped in RPMs. Relaxing bounds on dependencies can easily lead to us, developers, using new functionality that's not available to end users. Pint 0.24 may add the PlainQuantity.magnitude_but_better attribute, we start using it in code, and if we don't have the relevant code covered by functional tests, nothing will stop us from shipping it to CentOS Stream 9 where only Pint 0.16 is available.

Fair point, but if that is the case, mypy and pyright should be added to testing-farm tests, otherwise withing pre-commit we are not testing the specific versions either. My view with pre-commit is that it's for style checks and lint-checks against PyPI only.

---

But going back to the failures encountered here, it is more mysterious the more we look into it:

if TYPE_CHECKING:
    from .facets.plain import PlainQuantity as Quantity

...

class PlainQuantity(Generic[MagnitudeT], PrettyIPython, SharedRegistryObject):

The previous syntax should never have failed 🤔

[happz]

A lot, I would say, because mypy and pyright are not just about type checking, they also detect attributes that are not defined, for example. Most tmt users depend on tmt that's packaged and shipped in RPMs. Relaxing bounds on dependencies can easily lead to us, developers, using new functionality that's not available to end users. Pint 0.24 may add the PlainQuantity.magnitude_but_better attribute, we start using it in code, and if we don't have the relevant code covered by functional tests, nothing will stop us from shipping it to CentOS Stream 9 where only Pint 0.16 is available.

Fair point, but if that is the case, mypy and pyright should be added to testing-farm tests, otherwise withing pre-commit we are not testing the specific versions either.

Yeah, that's what the last two paragraphs in my previous comment were about - I think we need this to happen Soon before it bites us after a random release :)

But going back to the failures encountered here, it is more mysterious the more we look into it:

if TYPE_CHECKING:
    from .facets.plain import PlainQuantity as Quantity

...

class PlainQuantity(Generic[MagnitudeT], PrettyIPython, SharedRegistryObject):

The previous syntax should never have failed 🤔

I'll check it out after lunch, and try to understand it better. I recall seeing these new classes pop up, but that's pretty much all.

[lukaszachy]

+1 to have mypy/pyright in testing farm.

As for upper bounds everywhere - We could add it but we will need to update it quite often to not block adoption of newer versions into fedora-rawhide. So I'm not sure what the benefit will be.

Could we add some testing-farm installing from pipy? Maybe a weekly/nighly just to check what are the latest pypi versions and how we work with them?

[LecrisUT]

Could we add some testing-farm installing from pipy? Maybe a weekly/nighly just to check what are the latest pypi versions and how we work with them?

Let's wait for #2557 and figure out a design after that. The current plans are getting quite a bit messy imo, and they should be divided in more matrix-like approach, e.g. /plans/dist/integration, /plans/pypi/integration, etc.

One approach I was considering for tackling that is to use jinja templating to generate the /plan/*.fmf file structure and then pipe that into testing-farm/packit. I saw there is some jinja templating, but I didn't check where that is applied though. A simple generator for that would be this one, but I need to simplify and make it more fmf native.

Another approach would be to use plans.import + environment, e.g. /plans/dist import /plans/integration, but that would be tricky because it cannot work with context right now, or to add/remove prepare tasks which is what we would want here.

[thrix]

I read the whole discussion and also appear this is a smoking barrel that can easily blow up if we do not start runinng mypy/pyright in testing farm. Something maybe to remember for the future, how it can hide problems

[happz]

As for upper bounds everywhere - We could add it but we will need to update it quite often to not block adoption of newer versions into fedora-rawhide. So I'm not sure what the benefit will be.

Probably not needed most of the time. There might be buggy versions or incompatible versions we'd like to avoid, (click>=8.0.3,!=8.1.4), and Pint was an example in which the upper bound prevented a code that's harder to mend with existing code, and needs more work and a bigger PR, but in general, probably not necessary.

[LecrisUT]

No idea why pre-commit decided to fail in this PR, maybe it runs on different files in a commit vs merge. Anyway patched those up in this PR

[happz]

Those unrelated changes are really ugly... Why would they appear here and now when this morning's pre-commit output complained about nothing to me, no idea. Your patch does not even touch those files, meh.

[happz]

Those unrelated changes are really ugly... Why would they appear here and now when this morning's pre-commit output complained about nothing to me, no idea. Your patch does not even touch those files, meh.

I'd make a patch fixing just those, I wanted to update pre-commit checks anyway, let's fix those there, then we can rebase your PR and keep the diff simpler.

[LecrisUT]

I'd make a patch fixing just those, I wanted to update pre-commit checks anyway, let's fix those there, then we can rebase your PR and keep the diff simpler.

👍

Btw, regarding 1.35 milestone, since this is blocking xarray upgrade this might get patched in rawhide as soon as 1.34 is released. Let's at least get the tests running (needs /packit build and maybe /packit test later) to give them the green light to unblock pint

[happz]

FTR, here's the update of pre-commit checks, it does cover changes seen in this PR plus some more: #3023

[happz]

/packit build

[LecrisUT]

I don't see /tests/unit/with-development-packages being run anywhere there.

Those unrelated changes are really ugly... Why would they appear here and now when this morning's pre-commit output complained about nothing to me, no idea.

Oh, maybe touching pyproject.toml made it trigger on all files

[LecrisUT]

Greaat... We need a better approach for this.

The test is behind /plans/features/core which was gated by:

tmt/.packit.yaml

Lines 43 to 45 in 87f2fe0

	targets:
	- fedora-latest-stable
	- epel-9

[happz]

I don't see /tests/unit/with-development-packages being run anywhere there.

That's weird, this test should be there, in the testing-farm:fedora-39-x86_64:full check. Can you undo the change? I will hit Packit once again.

[LecrisUT]

Ah right, I've only checked F40, F39 is indeed 👍

[happz]

/packit build