Skip to content
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

Some test dependencies have no lower bounds #42989

Closed
30 tasks done
potiuk opened this issue Oct 14, 2024 · 12 comments · Fixed by #43603
Closed
30 tasks done

Some test dependencies have no lower bounds #42989

potiuk opened this issue Oct 14, 2024 · 12 comments · Fixed by #43603
Assignees
Labels
area:dependencies Issues related to dependencies problems

Comments

@potiuk
Copy link
Member

potiuk commented Oct 14, 2024

Some test dependencies have no "lower bounds" - this is not very good because "lowest-direct" dependency resolution might run much longer. Currently when uv is making the resolution, it prints warnings

  • warning: Missing version constraint (e.g., a lower bound) for asgiref
  • warning: Missing version constraint (e.g., a lower bound) for requests-toolbelt
  • warning: Missing version constraint (e.g., a lower bound) for cloudpickle
  • warning: Missing version constraint (e.g., a lower bound) for python-ldap
  • warning: Missing version constraint (e.g., a lower bound) for plyvel
  • warning: Missing version constraint (e.g., a lower bound) for opentelemetry-exporter-prometheus
  • warning: Missing version constraint (e.g., a lower bound) for amqp
  • warning: Missing version constraint (e.g., a lower bound) for virtualenv
  • warning: Missing version constraint (e.g., a lower bound) for types-markdown
  • warning: Missing version constraint (e.g., a lower bound) for types-deprecated
  • warning: Missing version constraint (e.g., a lower bound) for types-pymysql
  • warning: Missing version constraint (e.g., a lower bound) for types-pyyaml
  • warning: Missing version constraint (e.g., a lower bound) for types-aiofiles
  • warning: Missing version constraint (e.g., a lower bound) for types-certifi
  • warning: Missing version constraint (e.g., a lower bound) for types-croniter
  • warning: Missing version constraint (e.g., a lower bound) for types-docutils
  • warning: Missing version constraint (e.g., a lower bound) for types-paramiko
  • warning: Missing version constraint (e.g., a lower bound) for types-protobuf
  • warning: Missing version constraint (e.g., a lower bound) for types-python-dateutil
  • warning: Missing version constraint (e.g., a lower bound) for types-python-slugify
  • warning: Missing version constraint (e.g., a lower bound) for types-pytz
  • warning: Missing version constraint (e.g., a lower bound) for types-redis
  • warning: Missing version constraint (e.g., a lower bound) for types-requests
  • warning: Missing version constraint (e.g., a lower bound) for types-setuptools
  • warning: Missing version constraint (e.g., a lower bound) for types-tabulate
  • warning: Missing version constraint (e.g., a lower bound) for types-termcolor
  • warning: Missing version constraint (e.g., a lower bound) for types-toml
  • warning: Missing version constraint (e.g., a lower bound) for ipykernel
  • warning: Missing version constraint (e.g., a lower bound) for pywinrm
  • warning: Missing version constraint (e.g., a lower bound) for scrapbook

We should make sure that all those dependencies have reasonable lower-bounds . Ideally versions that are not too old (6 months?). We shoudl look at them individually though, to see if there might be potential issues when we are choosing the lower bounds.

@potiuk potiuk converted this from a draft issue Oct 14, 2024
@dosubot dosubot bot added the area:dependencies Issues related to dependencies problems label Oct 14, 2024
@rawwar
Copy link
Collaborator

rawwar commented Oct 14, 2024

@potiuk , I think, "lower bound" was given as an example in the warnings.~~

For example, asgiref is currently set to be "asgiref>=2.3.0", in hatch_build.py.

Looks like, the issue is with constraints.txt and that's where we need to put the lowerbound

@potiuk, I'm a bit confused here. All of the pins in constraints.txt use exact matches (==). Not sure where the issue is. I tried to set an upper bound to asgiref and yet uv is printing same warning

@potiuk
Copy link
Member Author

potiuk commented Oct 14, 2024

@potiuk , I think, "lower bound" was given as an example in the warnings.~~

For example, asgiref is currently set to be "asgiref>=2.3.0", in hatch_build.py.

Looks like, the issue is with constraints.txt and that's where we need to put the lowerbound

@potiuk, I'm a bit confused here. All of the pins in constraints.txt use exact matches (==). Not sure where the issue is. I tried to set an upper bound to asgiref and yet uv is printing same warning

Interesting. Worth taking a closer look. Constraints do not matter here. When we use "lowest-direct", we do not use constraints at all. Those are only used when we (or our users) want to reproducibly install airflow, but for --resolution lowest-direct we do not specify constraints (and it would make no sense) - we let uv lower all dependencies matching the requirements.

I think the asgiref warning comes from http provider dependencies (see provider.yaml for http provider)

dependencies:
  - apache-airflow>=2.8.0
  # The 2.26.0 release of requests got rid of the chardet LGPL mandatory dependency, allowing us to
  # release it as a requirement for airflow
  - requests>=2.27.0,<3
  - requests_toolbelt
  - aiohttp>=3.9.2
  - asgiref

I think fix for that particular one will be to make sure all asgiref dependencies we have use the same lower-bound.

BTW. It's also surprising to see asgiref as http provider dependencies - so maybe we should take a look if it is needed at all @rawwar :) ?

@potiuk
Copy link
Member Author

potiuk commented Oct 14, 2024

BTW. I see those warnings as a great opportunity to guide us with reviewing some of our dependencies - because like in this case we might have some surprising and not entirely proper dependencies configured in some of our providers :)

@kaxil
Copy link
Member

kaxil commented Oct 14, 2024

BTW. I see those warnings as a great opportunity to guide us with reviewing some of our dependencies - because like in this case we might have some surprising and not entirely proper dependencies configured in some of our providers :)

Agreed

@rawwar
Copy link
Collaborator

rawwar commented Oct 14, 2024

I think the asgiref warning comes from http provider dependencies (see provider.yaml for http provider)

I tried this in the PR: #43001 and it still prints warning for asgiref.

BTW. It's also surprising to see asgiref as http provider dependencies - so maybe we should take a look if it is needed at all @rawwar :) ?

We only use it here(Link) .

I will look into it and see if its possible to change this and remove the dependency.

@rawwar
Copy link
Collaborator

rawwar commented Oct 16, 2024

@potiuk , can you assign me this issue?

@potiuk
Copy link
Member Author

potiuk commented Oct 16, 2024

@potiuk , can you assign me this issue?

sure :)

BTW. I can add you to the triage team so that you can do it yourself (and help with triage/setting labels as well) - WDYT @rawwar ?

@potiuk
Copy link
Member Author

potiuk commented Oct 16, 2024

#43074 ?

@rawwar
Copy link
Collaborator

rawwar commented Oct 19, 2024

@potiuk , i think i misunderstood when you mentioned, "we need to look into packages individually, to avoid issues"

Can i raise PRs for miltiple package pins together? As long as tests pass, this should be fine right?

Raising a PR for each package separately will take a lot of time to finish this issue.

@jedcunningham
Copy link
Member

I don't see an issue with batching them.

@potiuk I'm slightly hesitant to set the minimum purely based on "~6 months old" though. Are we planning to moving those lower bounds for everything, on an ongoing basis?

If we do do it based on time alone (like in #43189 seemingly), we should definitely state there is no "real" reason for that being the minimum and we chose something arbitrary, for posterity sake.

@potiuk
Copy link
Member Author

potiuk commented Oct 24, 2024

I don't see an issue with batching them.

Me neither.

@potiuk I'm slightly hesitant to set the minimum purely based on "~6 months old" though. Are we planning to moving those lower bounds for everything, on an ongoing basis?

We already have a working solution to detect if we are starting to use features that is not available in "lowest" supported version - there is the "Lowest Dependency test" that does it. So this exercise is really to get "some" baselines on those dependencies. I think we we will generally bring them up usually when we start using something that does not pass test with the lowest possible versions brought down for all dependencies (of partucular provider or airflow core).

I am not tied to "6 months" but that sounded like a good baseline. If there is any other proposal how to get the baseline - I am happy to any reasonable proposal there @jedcunningham

And generally we avoid to document lower-bounds - it's generally not needed, we pretty much never go down. We must document upper-bounds, but as long as lower-binding works, it's generally ok - unless of course someone has conflicting upper bound limit. But I look at those deps that we miss lower binds, and I don't think there is high risk for that.

But if there is any other proposal - I am all ears.

@potiuk
Copy link
Member Author

potiuk commented Oct 24, 2024

BTW. We already have a number of some arbitrary lower-bounds - not documented - and I can't recall a single case where it cause problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dependencies Issues related to dependencies problems
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants