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 2020/09/07 19:09:01 UTC
[GitHub] [airflow] potiuk opened a new pull request #10784: Requirements might get upgraded without setup.py change
potiuk opened a new pull request #10784:
URL: https://github.com/apache/airflow/pull/10784
I noticed that when there is no setup.py changes, the constraints
are not upgraded automatically. This is because of the docker
caching strategy used - it simply does not even know that the
upgrade of pip should happen.
I believe this is really good (from security and incremental updates
POV to attempt to upgrade at every successfull merge (not that
the upgrade will not be committed if any of the tests fail and this
is only happening on every merge to master or scheduled run.
This way we will have more often but smaller constraint changes.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r488674590
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
This means that the PRs will run with the upgraded constraints right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490609617
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Everything will work, I believe (but it would be great to check if there are no loopholes).
This part has not changed and we run it for > year now. Only the location changed to run the scripts - the scripts are run in the "workflow_run". Here we build the image using `scripts/ci` taken from master, but the `Dockerfile`, `setup.py`, `scripts/in_container` and everything else is coming from the PR.
In Dockerfile.ci there are two steps of pip install:
1) The first one: https://github.com/apache/airflow/blob/5b3fb53d9c5c943abb8ddc90214c320b7a11c8c2/Dockerfile.ci#L223 installs deps from master. This one only invalidates when we rebuild the image from scratch (i.e. when we change Dockerfile or when we get a new base python image. In all other cases, this one is taken from cache - even if setup.py changes. This is because we COPY setup.py in one of the following lines (not before). It's really an optimization - thanks to that, even if setup.py changes, it does not take 20 minutes to build the image but maybe 6-7 minutes (because all dependencies from the previous master run are already installed in the layer taken from the cache.
2) then we copy setup.py: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L243 - if setup.py is not changed vs. latest master build - this layer is also taken from cache as well as the subsequent step of pip install (this way if no setup.py changes the image build is ~ 3/4 minutes - we do not lose time on even checking for new requirements. But if the setup.py changes (setup.py comes from the PR - just to remind) then Docker cache gets invalidated here and all later layers of the image are built from here.
3) Then if setup.py changed in the previous step we reach that line during rebuilding the layers: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L258.
In "normal" PR's (UPGRADE_TO_LATEST_CONSTRAINT = "false") we run this command:
`pip install -e ".[${AIRFLOW_EXTRAS}]" --constraint "${AIRFLOW_CONSTRAINTS_URL}" `
This will:
a) install all the new stuff that appeared in setup.py (so here where we install new packages)
b) upgrade all the stuff for which limits were changed in setup.py and the limits do not fit either constraints or already installed versions
c) but all the rest remains untouched.
This way PR gets only the previously "good" constrains + anything that changed in the setuup.py for this PR (this is where we avoid the 'package not found`).
Basically it means that it will only try to upgrade or install new deps that freshly appeared in setup.py. But if the previous requirements are fitting the setup.py limits, no "eager" upgrade of those requirements is performed.
In the other case (UPGRADE_TO_LATEST_CONSTRAINT = "true") the same pip install is run with `--upgrade --upgrade-strategy eager and NO constraints'. This is only run in case we run a "master merge" push or scheduled build. This means that pip will try to upgrade all the dependencies to their most recent versions that fulfill the setup.py limitations - without looking at the last constraints. If such a build succeeds (all tests pass), then we upgrade constraints to match those newly upgraded versions (because we know that they passed the tests). This is recorded as the commit message in teh orphaned "constraints-master" or constraints-1-10 branches (depending if it was a master or v1-10-test push):
Example here: https://github.com/apache/airflow/commit/625194596587bd8b3ef54f1902b6060e4a0c187b - you see which commit originated it and what exactly changes so we can easily track the history of upgrades of those.
Once the image is build (with potentially new requirements), it is pushed to registry and tests for the run that triggered the build are run using this image.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r488679641
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Nope. Only when the buld runs in master. All the PRs have "UPGRADE_TO_LATEST_CONSTRAINTS" set to "false" but all the "master" push-es have the UPGRADE_TO_LATEST_CONSTRAINTS set to "COMMIT_SHA" so that every new master push will automatically invalidate this line in Dockerfile (but only master push, not PR).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694568246
> Because the constraints were not upgraded (fixed by this PR) the Pandas upgrade also introduced a breaking change #11004 . So having the eager upgrade in push will actually help us to detect such breaking changes faster (BTW. it's great that both moto and pandas breaking changes have not impacted regular PRs so far = because of the design :) . We would be in much worse state if it was not implemented like that with all the PRs failing out of the sudden.).
I even tried on one of the forks to pass true by default and see `UPGRADE_TO_LATEST_CONSTRAINTS: "true"`, it still failed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694789020
Ok. looking at those. Maybe there is a bug somewhere :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10784:
URL: https://github.com/apache/airflow/pull/10784
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-691866560
Looks like it's going to be good-to-go
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r485144932
##########
File path: .github/workflows/ci.yml
##########
@@ -35,7 +35,7 @@ env:
SKIP_CHECK_REMOTE_IMAGE: "true"
DB_RESET: "true"
VERBOSE: "true"
- UPGRADE_TO_LATEST_CONSTRAINTS: ${{ github.event_name == 'push' || github.event_name == 'scheduled' }}
+ UPGRADE_TO_LATEST_CONSTRAINTS: ${{ githubgithub.event_name == 'push' || github.event_name == 'scheduled' }}
Review comment:
```suggestion
UPGRADE_TO_LATEST_CONSTRAINTS: ${{ github.event_name == 'push' || github.event_name == 'scheduled' }}
```
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -141,6 +142,15 @@ jobs:
else
echo "::set-output name=docker-cache::pulled"
fi
+ - name: "Set upgrade to latest constaints"
Review comment:
```suggestion
- name: "Set upgrade to latest constraints"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-693399977
> We need to pin moto==1.3.14 for now or fix the Datasync issues for this PR to pass the tests
Yeah. had no time to finish it but this was what I thought as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490609617
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Everything will work, I believe (but it would be great to check if there are no loopholes).
This part has not changed and we run it for > year now. Only the location changed to run the scripts - the scripts are run in the "workflow_run". Here we build the image using `scripts/ci` taken from master, but the `Dockerfile`, `setup.py`, `scripts/in_container` and everything else is coming from the PR.
In Dockerfile.ci there are two steps of pip install:
1) The first one: https://github.com/apache/airflow/blob/5b3fb53d9c5c943abb8ddc90214c320b7a11c8c2/Dockerfile.ci#L223 installs deps from master. This one only invalidates when we rebuild the image from scratch (i.e. when we change Dockerfile or when we get a new base python image. In all other cases, this one is taken from cache - even if setup.py changes. This is because we COPY setup.py in one of the following lines (not before). It's really an optimization - thanks to that, even if setup.py changes, it does not take 20 minutes to build the image but maybe 6-7 minutes (because all dependencies from the previous master run are already installed in the layer taken from the cache.
2) then we copy setup.py: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L243 - if setup.py is not changed vs. latest master build - this layer is also taken from cache as well as the subsequent step of pip install (this way if no setup.py changes the image build is ~ 3/4 minutes - we do not lose time on even checking for new requirements. But if the setup.py changes (setup.py comes from the PR - just to remind) then Docker cache gets invalidated here and all later layers of the image are built from here.
3) Then if setup.py changed in the previous step we reach that line during rebuilding the layers: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L258.
In "normal" PR's (UPGRADE_TO_LATEST_CONSTRAINT = "false") we run this command:
`pip install -e ".[${AIRFLOW_EXTRAS}]" --constraint "${AIRFLOW_CONSTRAINTS_URL}" `
This will:
a) install all the new stuff that appeared in setup.py (so here where we install new packages)
b) upgrade all the stuff for which limits were changed in setup.py and the limits do not fit either constraints or already installed versions
c) but all the rest remains untouched.
This way PR gets only the previously "good" constrains + anything that changed in the setuup.py for this PR (this is where we avoid the 'package not found`).
Basically it means that it will only try to upgrade or install new deps that freshly appeared in setup.py. But if the previous requirements are fitting the setup.py limits, no "eager" upgrade of those requirements is performed.
In the other case (UPGRADE_TO_LATEST_CONSTRAINT = "commit hash") the same pip install is run with `--upgrade --upgrade-strategy eager and NO constraints'. This is only run in case we run a "master merge" push or scheduled build. This means that pip will try to upgrade all the dependencies to their most recent versions that fulfill the setup.py limitations - without looking at the last constraints. If such a build succeeds (all tests pass), then we upgrade constraints to match those newly upgraded versions (because we know that they passed the tests). This is recorded as the commit message in teh orphaned "constraints-master" or constraints-1-10 branches (depending if it was a master or v1-10-test push):
Example here: https://github.com/apache/airflow/commit/625194596587bd8b3ef54f1902b6060e4a0c187b - you see which commit originated it and what exactly changes so we can easily track the history of upgrades of those.
Once the image is build (with potentially new requirements), it is pushed to registry and tests for the run that triggered the build are run using this image.
Why check-ing for "commit hash" - because if it was "true" then this line woudl only be invalidated if anything changed in the "setup.py" above.. By adding commit hash here, we make sure to invalidate this line with every merge push, and it means that we will be running "--upgrade --upgrade-strategy eager" every time we merge to master. This way we will have bigger number of smaller incremental changes (with newly released versions of dependent libraries" every time we merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-691842392
@potiuk I think when you do a rebase now it is good to go?!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-691853980
Ah yeah . let me try :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-689095091
Yeah. I noticed some unexpected behaviour with changes from yesterday and had to fix it first :).
Pushed changes now, I also test it on my own fork (it can only be fully tested in master)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490609617
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Everything will work, I believe (but it would be great to check if there are no loopholes).
This part has not changed and we run it for > year now. Only the location changed to run the scripts - the scripts are run in the "workflow_run". Here we build the image using `scripts/ci` taken from master, but the `Dockerfile`, `setup.py`, `scripts/in_container` and everything else is coming from the PR.
In Dockerfile.ci there are two steps of pip install:
1) The first one: https://github.com/apache/airflow/blob/5b3fb53d9c5c943abb8ddc90214c320b7a11c8c2/Dockerfile.ci#L223 installs deps from master. This one only invalidates when we rebuild the image from scratch (i.e. when we change Dockerfile or when we get a new base python image. In all other cases, this one is taken from cache - even if setup.py changes. This is because we COPY setup.py in one of the following lines (not before). It's really an optimization - thanks to that, even if setup.py changes, it does not take 20 minutes to build the image but maybe 6-7 minutes (because all dependencies from the previous master run are already installed in the layer taken from the cache.
2) then we copy setup.py: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L243 - if setup.py is not changed vs. latest master build - this layer is also taken from cache as well as the subsequent step of pip install (this way if no setup.py changes the image build is ~ 3/4 minutes - we do not lose time on even checking for new requirements. But if the setup.py changes (setup.py comes from the PR - just to remind) then Docker cache gets invalidated here and all later layers of the image are built from here.
3) Then if setup.py changed in the previous step we reach that line during rebuilding the layers: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L258.
In "normal" PR's (UPGRADE_TO_LATEST_CONSTRAINT = "false") we run this command:
`pip install -e ".[${AIRFLOW_EXTRAS}]" --constraint "${AIRFLOW_CONSTRAINTS_URL}" `
This will:
a) install all the new stuff that appeared in setup.py (so here where we install new packages)
b) upgrade all the stuff for which limits were changed in setup.py and the limits do not fit either constraints or already installed versions
c) but all the rest remains untouched.
This way PR gets only the previously "good" constrains + anything that changed in the setuup.py for this PR (this is where we avoid the 'package not found`).
Basically it means that it will only try to upgrade or install new deps that freshly appeared in setup.py. But if the previous requirements are fitting the setup.py limits, no "eager" upgrade of those requirements is performed.
In the other case (UPGRADE_TO_LATEST_CONSTRAINT = "commit hash") the same pip install is run with `--upgrade --upgrade-strategy eager and NO constraints'. This is only run in case we run a "master merge" push or scheduled build. This means that pip will try to upgrade all the dependencies to their most recent versions that fulfill the setup.py limitations - without looking at the last constraints. If such a build succeeds (all tests pass), then we upgrade constraints to match those newly upgraded versions (because we know that they passed the tests). This is recorded as the commit message in teh orphaned "constraints-master" or constraints-1-10 branches (depending if it was a master or v1-10-test push):
Example here: https://github.com/apache/airflow/commit/625194596587bd8b3ef54f1902b6060e4a0c187b - you see which commit originated it and what exactly changes so we can easily track the history of upgrades of those.
Once the image is build (with potentially new requirements), it is pushed to registry and tests for the run that triggered the build are run using this image.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10784:
URL: https://github.com/apache/airflow/pull/10784
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r488682696
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
So bottomline: Every "merge" and "scheduled" build will attempt to automatically upgrade to latest constratins. Additionally if a "merge" push will run successfully (i.e. all the tests pass) the newly upgraded constratins will be pushed to the "constraints" branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490615075
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Example Run with failure:
https://github.com/apache/airflow/runs/1130333784?check_suite_focus=true
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-696691198
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r488685014
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
And in case there is a problem with any dependencies we will see merge builds failing, but it will not impact the PRs. This so far happened only when setup.py changed, but I believe smaller incremental upgrades after every merge make much more sense as we will precisely see what caused the problems. I also plan to show a "constraint diff" in such build shortly soo that we immediately see which changed requirements caused failure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r485396573
##########
File path: setup.py
##########
@@ -698,7 +699,7 @@ def is_package_excluded(package: str, exclusion_list: List[str]):
'flask>=1.1.0, <2.0',
'flask-appbuilder>2.3.4,~=3.0',
'flask-caching>=1.3.3, <2.0.0',
- 'flask-login>=0.3, <1.0',
+ 'flask-login>=0.3, <0.5',
Review comment:
And here.
##########
File path: setup.py
##########
@@ -201,6 +201,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'celery~=4.4.2',
'flower>=0.7.3, <1.0',
'tornado>=4.2.0, <6.0', # Dep of flower. Pin to a version that works on Py3.5.2
+ 'vine~=1.3', # https://stackoverflow.com/questions/32757259/celery-no-module-named-five
Review comment:
I read about it in your slack thread but this should be a separate PR imo.
##########
File path: setup.py
##########
@@ -247,7 +248,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'facebook-business>=6.0.2',
]
flask_oauth = [
- 'Flask-OAuthlib>=0.9.1',
+ 'Flask-OAuthlib>=0.9.1,<0.9.6', # Flask OAuthLib 0.9.6 requires Flask-Login 0.5.0 - breaks FAB
Review comment:
Same here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r488680540
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Correction. Every "push" and "scheduled" build as well: https://github.com/apache/airflow/pull/10784/files#diff-cf2e198c11119438802588d2da4d372aR145
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-696691198
Pushed it on top of the latest setup.py merges.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490614575
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
That was my impression and understanding too but when it failed `azure-identity` few days back and today again when I pushed that commit to v1-10-test the commit failed with `module not found`.
(btw thanks again for the detailed explanation)
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
That was my impression and understanding too but when it failed `azure-identity` few days back and today again when I pushed that commit to v1-10-test the commit failed with `module not found`.
(btw thanks for the detailed explanation)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r485539642
##########
File path: setup.py
##########
@@ -201,6 +201,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'celery~=4.4.2',
'flower>=0.7.3, <1.0',
'tornado>=4.2.0, <6.0', # Dep of flower. Pin to a version that works on Py3.5.2
+ 'vine~=1.3', # https://stackoverflow.com/questions/32757259/celery-no-module-named-five
Review comment:
Agree
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490614575
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
That was my impression and understanding too but when it failed `azure-identity` few days back and today again when I pushed that commit to v1-10-test the commit failed with `module not found`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490609617
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
Everything will work, I believe (but it would be great to check if there are no loopholes).
This part has not changed and we run it for > year now. Only the location changed to run the scripts - the scripts are run in the "workflow_run". Here we build the image using `scripts/ci` taken from master, but the `Dockerfile`, `setup.py`, `scripts/in_container` and everything else is coming from the PR.
In Dockerfile.ci there are two steps of pip install:
1) The first one: https://github.com/apache/airflow/blob/5b3fb53d9c5c943abb8ddc90214c320b7a11c8c2/Dockerfile.ci#L223 installs deps from master. This one only invalidates when we rebuild the image from scratch (i.e. when we change Dockerfile or when we get a new base python image. In all other cases, this one is taken from cache - even if setup.py changes. This is because we COPY setup.py in one of the following lines (not before). It's really an optimization - thanks to that, even if setup.py changes, it does not take 20 minutes to build the image but maybe 6-7 minutes (because all dependencies from the previous master run are already installed in the layer taken from the cache.
2) then we copy setup.py: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L243 - if setup.py is not changed vs. latest master build - this layer is also taken from cache as well as the subsequent step of pip install (this way if no setup.py changes the image build is ~ 3/4 minutes - we do not lose time on even checking for new requirements. But if the setup.py changes (setup.py comes from the PR - just to remind) then Docker cache gets invalidated here and all later layers of the image are built from here.
3) Then if setup.py changed in the previous step we reach that line during rebuilding the layers: https://github.com/apache/airflow/blob/master/Dockerfile.ci#L258.
In "normal" PR's (UPGRADE_TO_LATEST_CONSTRAINT = "false") we run this command:
`pip install -e ".[${AIRFLOW_EXTRAS}]" --constraint "${AIRFLOW_CONSTRAINTS_URL}" `
This will:
a) install all the new stuff that appeared in setup.py (so here where we install new packages)
b) upgrade all the stuff for which limits were changed in setup.py and the limits do not fit either constraints or already installed versions
c) but all the rest remains untouched.
This way PR gets only the previously "good" constrains + anything that changed in the setuup.py for this PR (this is where we avoid the 'package not found`).
Basically it means that it will only try to upgrade or install new deps that freshly appeared in setup.py. But if the previous requirements are fitting the setup.py limits, no "eager" upgrade of those requirements is performed.
In the other case (UPGRADE_TO_LATEST_CONSTRAINT = "commit hash") the same pip install is run with `--upgrade --upgrade-strategy eager and NO constraints'. This is only run in case we run a "master merge" push or scheduled build. This means that pip will try to upgrade all the dependencies to their most recent versions that fulfill the setup.py limitations - without looking at the last constraints. If such a build succeeds (all tests pass), then we upgrade constraints to match those newly upgraded versions (because we know that they passed the tests). This is recorded as the commit message in teh orphaned "constraints-master" or constraints-1-10 branches (depending if it was a master or v1-10-test push):
Example here: https://github.com/apache/airflow/commit/625194596587bd8b3ef54f1902b6060e4a0c187b - you see which commit originated it and what exactly changes so we can easily track the history of upgrades of those.
Once the image is build (with potentially new requirements), it is pushed to registry and tests for the run that triggered the build are run using this image.
Why check-ing for "commit hash" - because if it was "true" then this line woudl only be invalidated if anything changed in the "setup.py" above.. By adding commit hash here, we make sure to invalidate this line with every merge push, and it means that we will be running "--upgrade --upgrade-strategy" every time we merge to master. This way we will have bigger number of smaller incremental changes (with newly released versions of dependent libraries" every time we merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694569981
I have pushed the same thing to v1-10-test now, let's see if it works but also let me know if I am doing something wrong
https://github.com/apache/airflow/commits/v1-10-test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-692715399
@feluelle @kaxil -> seems that we have some new packages released that break our tests. From the first look it is pandas and AWS. I am glad we have the safety net in place.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r490597759
##########
File path: Dockerfile.ci
##########
@@ -256,7 +256,7 @@ ENV UPGRADE_TO_LATEST_CONSTRAINTS=${UPGRADE_TO_LATEST_CONSTRAINTS}
# But in cron job we will install latest versions matching setup.py to see if there is no breaking change
# and push the constraints if everything is successful
RUN \
- if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" == "true" ]]; then \
+ if [[ "${UPGRADE_TO_LATEST_CONSTRAINTS}" != "false" ]]; then \
Review comment:
What happens in case where we upgrade the dependencies in PR, will they fail with no package found error?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694560237
Because the constraints were not upgraded (fixed by this PR) the Pandas upgrade also introduced a breaking change #11004 . So having the eager upgrade in push will actually help us to detect such breaking changes faster (BTW. it's great that both moto and pandas breaking changes have not impacted regular PRs so far = because of the design :) . We would be in much worse state if it was not implemented like that with all the PRs failing out of the sudden.).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-696755122
Also 137 Memory problem . We need to do something about it ..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694569981
I have pushed the same thing to v1-10-test now, let's see if it works but also let me know if I am doing something wrong
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#issuecomment-694232138
The moto change is pinned now @kaxil :). I hope we get this one green this time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10784: Requirements might get upgraded without setup.py change
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10784:
URL: https://github.com/apache/airflow/pull/10784#discussion_r485543758
##########
File path: setup.py
##########
@@ -201,6 +201,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version
'celery~=4.4.2',
'flower>=0.7.3, <1.0',
'tornado>=4.2.0, <6.0', # Dep of flower. Pin to a version that works on Py3.5.2
+ 'vine~=1.3', # https://stackoverflow.com/questions/32757259/celery-no-module-named-five
Review comment:
Separated it out to https://github.com/apache/airflow/pull/10828 with more explanation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org