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

Turn optional-dependencies in pyproject.toml into dynamic property #38437

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 24, 2024

While currently hatchling and pip nicely supports dynamic replacement of the dependencies even if they are statically defined, this is not proper according to EP 621. When property of the project is set to be dynamic, it also contains static values. It's either static or dynamic.

This is not a problem for wheel packages when installed, by any standard tool, because the wheel package has all the metadata added to wheel (and does not contain pyproject.toml) but in various cases (such as installing airflow via Github URL or from sdist, it can make a difference - depending whether the tool installing airflow will use directly pyproject.toml for optimization, or whether it will run build hooks to prepare the dependencies).

This change makes all optional dependencies dynamici - rather than bake them in the pyproject.toml, we mark them as dynamic, so that any tool that uses pyproject.toml or sdist PKG-INFO will know that it has to run build hooks to get the actual optional dependencies.

There are a few consequences of that:

  • our pyproject.toml will not contain automatically generated part - which is actually good, as it caused some confusion

  • all dynamic optional dependencies of ours are either present in hatch_build.py or calculated there - this is a bit new but sounds reasonable - and those dynamic dependencies are not really updated often, so thish is not an issue to maintain them there

  • the pre-commits that manage the optional dependencies got a lot simpler now - a lot of code has been removed.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:dev-tools area:production-image Production image improvements and fixes kind:documentation labels Mar 24, 2024
@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch 5 times, most recently from ad76bf5 to db109c8 Compare March 24, 2024 15:52
@potiuk potiuk marked this pull request as ready for review March 24, 2024 15:52
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

After some recent (and looking and old) discussions, it turns out that we need one more thing to be fully PEP-517 compliant. While hatchling allows us (for now) to dynamically modiy project attributes that are "static" in pyproject.toml - this is not what it is supposed to happen, because various tools might take such dependencies directly from pyproject.toml if they are not marked as dynamic (and will not run the build hook to retrieve the dynamic values for those properties).

So we either generate the requiremets/optional dependencies dynamically, or we describe it statically in pyproject.toml, but we should not combine the two cases.

Comment describing it is here: pypa/pip#11440 (comment)

And I realized that after this discussion in this discussion in hatch pypa/hatch#1331 but also this comment in uv astral-sh/uv#2475 (comment).

Seems that what we do is a hack.

While it is convenient to keep dynamic requirements and optional dependencies in pyproject.toml, it seems that it is non-standard and can behave differently for different ways of installing airflow (from wheel file, sdist, url - depending what different tools are doing) - for one now pip installs current airflow "properly" from URL, while UV uses only pyproject.toml.

This had no impact on released airflow in wheel package, because wheel keeps dependenceis in METADATA, and this is where tools are getting the dependencies.

@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch from db109c8 to 8343dab Compare March 24, 2024 16:09
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

cc: @uranusjr -> I'd appreciate your comments here, I believe now we should be really compliant with the PEP expectations.

I compared the generated METADATA in the .whl files and they are very similar (some duplicate entrires removed, and I also got rid of the devel-ci from .whl - it's not needed there at all after using --editable install from downloaded package in the CI Dockerfile.

@jscheffl
Copy link
Contributor

I tried to "naively" install as described in my devenv.

  1. Build local CI Image: works
  2. Install via pip in local env: FAILS (see 1)
  1. Details of pip install in Xubuntu 22.04 x64:
(.venv-breeze) jscheffl@hp860g9:~/Workspace/airflow$ python3 -m venv PATH_TO_YOUR_VENV
(.venv-breeze) jscheffl@hp860g9:~/Workspace/airflow$ source PATH_TO_YOUR_VENV/bin/activate
(PATH_TO_YOUR_VENV) (.venv-breeze) jscheffl@hp860g9:~/Workspace/airflow$ pip install -e ".[devel]"
Obtaining file:///home/jscheffl/Workspace/airflow
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build editable ... done
  Installing backend dependencies ... done
  Preparing editable metadata (pyproject.toml) ... done
WARNING: apache-airflow 2.9.0.dev0 does not provide the extra 'devel'
Building wheels for collected packages: apache-airflow
  Building editable for apache-airflow (pyproject.toml) ... done
  Created wheel for apache-airflow: filename=apache_airflow-2.9.0.dev0-py3-none-any.whl size=67494 sha256=a2a230edd149efea5937bcc8f9e857ce1a65b349ce365cfb3d5da2777b491a66
  Stored in directory: /tmp/pip-ephem-wheel-cache-rgbnftf3/wheels/82/15/4a/bc39417e37a8668eb5fd65e3ae91c2adaba2a1bb006639f92e
Successfully built apache-airflow
Installing collected packages: apache-airflow
Successfully installed apache-airflow-2.9.0.dev0
(PATH_TO_YOUR_VENV) (.venv-breeze) jscheffl@hp860g9:~/Workspace/airflow$ pip list
Package        Version    Editable project location
-------------- ---------- --------------------------------
apache-airflow 2.9.0.dev0 /home/jscheffl/Workspace/airflow
pip            22.0.2
setuptools     59.6.0

...seems besides the airflow package no other dependencies are installed.

@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

Yes. You must upgrade your pip. Version 22 is from 2022 and a lot of the functionality and PEPs needed have been approved and implemented later than that.

@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

General advice from pip team is to always upgrade to newest version.

@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch 2 times, most recently from e113944 to 902d144 Compare March 24, 2024 18:05
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

Actually looks like pip 22.0 is the LAST one that does not support this dynamic retrieval of dependencies. So you picked really edge'y case @jscheffl. I will add this to the docs.

@jscheffl
Copy link
Contributor

Actually looks like pip 22.0 is the LAST one that does not support this dynamic retrieval of dependencies. So you picked really edge'y case @jscheffl. I will add this to the docs.

That is why it is good to test :-D

If I run into this we should handle this explicitly. Is it possible to fail install if pip version is too old?

@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch from 902d144 to 2b518b4 Compare March 24, 2024 18:20
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

If I run into this we should handle this explicitly. Is it possible to fail install if pip version is too old?

No. But it ONLY affects you if you want to install airflow from sources for development. It should not affect wheel installation, because generally this change does not impact the way how METADATA is stores in the .wheel - this happens during hatch build so it really affects the "builders" not "users" of the packages and local contributors.

@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

Updated documentation

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further naive tests and pip updates I can confirm local cases are working.

INSTALL Show resolved Hide resolved
@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch from 2b518b4 to 02a6ee9 Compare March 24, 2024 19:15
While currently hatchling and pip nicely supports dynamic replacement of
the dependencies even if they are statically defined, this is not proper
according to EP 621. When property of the project is set to be dynamic,
it also contains static values. It's either static or dynamic.

This is not a problem for wheel packages when installed, by any
standard tool, because the wheel package has all the metadata added
to wheel (and does not contain pyproject.toml) but in various cases
(such as installing airflow via Github URL or from sdist, it can
make a difference - depending whether the tool installing airflow will
use directly pyproject.toml for optimization, or whether it will run
build hooks to prepare the dependencies).

This change makes all optional dependencies dynamici - rather than
bake them in the pyproject.toml, we mark them as dynamic, so that
any tool that uses pyproject.toml or sdist PKG-INFO will know that
it has to run build hooks to get the actual optional dependencies.

There are a few consequences of that:

* our pyproject.toml will not contain automatically generated
  part - which is actually good, as it caused some confusion

* all dynamic optional dependencies of ours are either present in
  hatch_build.py or calculated there - this is a bit new
  but sounds reasonable - and those dynamic dependencies are not
  really updated often, so thish is not an issue to maintain them
  there

* the pre-commits that manage the optional dependencies got a lot
  simpler now - a lot of code has been removed.
@potiuk potiuk force-pushed the turn-optional-dependencies-in-dynamic-metadata branch from 02a6ee9 to 8313523 Compare March 24, 2024 19:16
@potiuk
Copy link
Member Author

potiuk commented Mar 24, 2024

Ah. Now with traceback, it's clear. The old breeze expects "dependencies" property in project ... That's fine. This will be fixed once the PR is merged.

@potiuk potiuk merged commit c883703 into main Mar 24, 2024
82 of 84 checks passed
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 25, 2024
@potiuk potiuk deleted the turn-optional-dependencies-in-dynamic-metadata branch October 1, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:dev-tools area:production-image Production image improvements and fixes changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants