You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/03/12 15:18:20 UTC
[GitHub] [airflow] potiuk opened a new pull request #22204: Add description on the vendoring process we use
potiuk opened a new pull request #22204:
URL: https://github.com/apache/airflow/pull/22204
We need to vendor in cgroupspy library in order to make Airflow
compatible with Python 3.10 (see #22050) so this is the right time
to make our vendoring process more organized.
I based in parts on the readme described by `bleach` package.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066095318
The Fix to cgroupspy submitted: https://github.com/cloudsigma/cgroupspy/pull/14 - but the last change was 7 months ago, so I am not sure how fast it might get merged/released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066092090
> Not sure if Airflow needs this much infrastructure (we only vendor two? dependencies and don’t generally upgrade the existing one), but could be useful if we want to go this route.
Yeah. I considered it too, but let's say we might want to do it when we see if it is needed. For now we have `0` vendored-in dependenciesand and the aim is to remove them asap - likely with next release (I am going to submit a PR to fix it in cgroupspy in a moment).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066092090
> Not sure if Airflow needs this much infrastructure (we only vendor two? dependencies and don’t generally upgrade the existing one), but could be useful if we want to go this route.
Yeah. I considered it too, but let's say we might want to do it when we see if it is needed. For now we have `0` vendored-in dependencies and and the aim is to remove them asap - likely with next release (I am going to submit a PR to fix it in cgroupspy in a moment).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1070536195
🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r829015737
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airflow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the `_vendor` package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling (so that we do not have to release multi-platform airflow packages on PyPI).
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
Review comment:
```suggestion
6. No modifications to the library may be made in the initial commit (except import paths).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1073122256
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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066013806
Some ideas from how pip does this. pip fully automates the vendoring-patching process so a new version can be pulled immediately if needed. It uses [vendoring](https://pypi.org/project/vendoring/) for this. It takes a requirements file (a la requiurements.txt) and diff files for patches. When invoked, it downloads and extracts the package and applies the patches in the vendor directory (specified in pyproject.toml).
Not sure if Airflow needs this much infrastructure (we only vendor two? dependencies and don’t generally upgrade the existing one), but could be useful if we want to go this route.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #22204:
URL: https://github.com/apache/airflow/pull/22204
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r825371901
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airlfow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the _vendor package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling.
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
+7. Apply the fixes necessary to use the vendored library as separate commits - each package separately,
+ so that they can be cherry-picked later if we upgrade the vendored package. Changes to airflow code to
+ use the vendored packages should be applied as separate commits/PRs.
+8. The `_vendor` packages should be excluded from any refactorings, static checks and automated fixes.
+
+## Adding and upgrading a vendored package
+
+Way to vendor a library or update a version:
+
+1. Update ``vendor.txt`` with the library, version, and SHA256 (`pypi` provides gashes as of recently)
Review comment:
Should this be `vendor.md`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1067146924
Hey @kaxil I cherry-picked your process description fixes from #22209 and added more description on the "no-compile".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r829279492
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airflow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the `_vendor` package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling (so that we do not have to release multi-platform airflow packages on PyPI).
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
Review comment:
The import paths should be separate commit -> this way it is easier to cherry pick changes later. This is what I've done to show it:
* https://github.com/apache/airflow/pull/22206 -> adding cgroups
* Fix imports: https://github.com/apache/airflow/pull/22207
This will allow to replace code with the next version (equivalent of #2206) and then apply the import change cherry-pick (#22207) in case we need ot upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1068270058
Hey @kaxil -> can we merge it ? I hope to start merging the series of PRs: https://github.com/apache/airflow/pull/22204, https://github.com/apache/airflow/pull/22206, https://github.com/apache/airflow/pull/22207, https://github.com/apache/airflow/pull/22208, https://github.com/apache/airflow/pull/22209 leading to Python 3.10 compatibility on Monay (hopefully) when I releae all the provider packages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066095852
> Yeah. I considered it too, but let's say we might want to do it when we see if it is needed. For now we have `0` vendored-in dependencies and and the aim is to remove them asap - likely with next release (I am going to submit a PR to fix it in cgroupspy in a moment).
I tihnk when we have more than one vendored in dependencies with significant changes and active development of the vendored-library (and when we see that such vendoring-in might be needed for at least few releases - we should consider automating it - in the case we have now with cgroupspy - I can't imagine they release new version without accepting the single `abc` import change, so I think any automation around it is a waste :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1066180414
First in-line to get finally Python 3.10 support :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r825441364
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airlfow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the _vendor package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling.
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
+7. Apply the fixes necessary to use the vendored library as separate commits - each package separately,
+ so that they can be cherry-picked later if we upgrade the vendored package. Changes to airflow code to
+ use the vendored packages should be applied as separate commits/PRs.
+8. The `_vendor` packages should be excluded from any refactorings, static checks and automated fixes.
+
+## Adding and upgrading a vendored package
+
+Way to vendor a library or update a version:
+
+1. Update ``vendor.txt`` with the library, version, and SHA256 (`pypi` provides gashes as of recently)
Review comment:
```suggestion
1. Update ``vendor.md`` with the library, version, and SHA256 (`pypi` provides gashes as of recently)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r829279492
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airflow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the `_vendor` package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling (so that we do not have to release multi-platform airflow packages on PyPI).
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
Review comment:
The import paths should be separate commit -> this way it is easier to cherry pick changes later. This is what I've done to show it:
* https://github.com/apache/airflow/pull/22206 -> adding cgroups
* Fix imports: https://github.com/apache/airflow/pull/22207
This will allow to replace code with the next version (equivalent of #2206) and then apply the import change cherry-pick (#22207) in case we need ot upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#discussion_r829015737
##########
File path: airflow/_vendor/README.md
##########
@@ -0,0 +1,37 @@
+# Vendor package
+
+## What vendored packages are for
+
+The `airflow._vendor` package is foreseen for vendoring in packages, that we have to modify ourselves
+because authors of the packages do not have time to modify them themselves. This is often temporary
+and once the packages implement fixes that we need, and then we remove the packages from
+the `_vendor` package.
+
+All Vendored libraries must follow these rules:
+
+1. Vendored libraries must be pure Python--no compiling (so that we do not have to release multi-platform airflow packages on PyPI).
+2. Source code for the libary is included in this directory.
+3. License must be included in this repo and in the [LICENSE](../../LICENSE) file and in the
+ [licenses](../../licenses) folder.
+4. Requirements of the library become requirements of airflow core.
+5. Version of the library should be included in the [vendor.md](vendor.md) file.
+6. No modifications to the library may be made in the initial commit.
Review comment:
```suggestion
6. No modifications to the library may be made in the initial commit (except import paths).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #22204:
URL: https://github.com/apache/airflow/pull/22204
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22204: Add description on the vendoring process we use
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22204:
URL: https://github.com/apache/airflow/pull/22204#issuecomment-1073112578
I rebased on top of latest changes - I need those https://github.com/apache/airflow/pull/22204, https://github.com/apache/airflow/pull/22206, https://github.com/apache/airflow/pull/22207, https://github.com/apache/airflow/pull/22208, https://github.com/apache/airflow/pull/22209 to complete Python 3.10 support
Looking for approvals on all of those (they are slal small)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org