-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add conditional version retrieval from setup. #12853
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
Add conditional version retrieval from setup. #12853
Conversation
When airflow is not installed as package (for example for local development from sources) there is no package metadata. Many of our unit tests use the version field and they fail if they are run within virtual environment where airflow is not installed as package (for example in IntelliJ this is default setting. This PR adds fall-back to read airflow version from setup in case it cannot be read from package metadata.
|
|
||
| log = logging.getLogger(__name__) | ||
| log.warning("Package metadata could not be found. Overriding it with version found in setup.py") | ||
| from setup import version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this logging necessary? Is there a possibility that the version may differ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a protection against the case when you think you have airflow installed from the package (i.e. in production) but you actually don't (for example you have it installed with pip -e .).
I think all production installations should be installed from packages (this is what I am working on in #12685. So the warning is deliberate here to be warned if that's the case.
Of course you will see in development as well, But I am not too worried about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better safe than sorry.
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we hardcoded Airflow Version in this file. Not sure when this changed. Will take a look when I am on laptop.
Feel free to merge, as my question does not actually is concerned with the change in this PR
It was change by @ashb last week (following my previous change that I added pre-commit to sync between the two).. |
When airflow is not installed as package (for example for local
development from sources) there is no package metadata.
Many of our unit tests use the version field and they fail if they
are run within virtual environment where airflow is not installed
as package (for example in IntelliJ this is default setting.
This PR adds fall-back to read airflow version from setup in
case it cannot be read from package metadata.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.