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/07/06 00:06:16 UTC

[GitHub] [airflow] potiuk opened a new pull request, #24858: Avoid running build ARM image twice

potiuk opened a new pull request, #24858:
URL: https://github.com/apache/airflow/pull/24858

In case of "merge-run" (i.e. when maintainer merges somethin
to main or v2-*-test branch is pushed, the ARM instance does not
need to be built because in such case cache is built and pushed
for ARM image.

<!--
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 an 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 changes, an 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 a newsfragment file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).

--
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] jedcunningham commented on pull request #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

jedcunningham commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1175622656

   Maybe I'm misunderstanding this, but doesn't this assume it passed and actually pushed the cache? What if it didn't and was merged anyways?


-- 
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1181186639

   Anyone? 


-- 
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1178176686

   @jedcunningham :)? 


-- 
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1176802577

   BTW. Fix to failing AMD cache build visible is also ready for review (green) in #24875 


-- 
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk merged PR #24858:
URL: https://github.com/apache/airflow/pull/24858


-- 
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1175955271

This is really an optimisation in case we have another ARM build already running (pushing cache).

Context:

Currently we have set of optimisation that only build ARM image when we suspect it might break (i.e when we have changes in dependencies - setup.py has been changed, any of the provider.yaml files changed). I think we should also add it when Dockerfile* change, but I will add it when I will change the Dockerfile next time (this is happening very rarely recently as it has gotten very stable).

Since - at least for now - we are only building the ARM image to verify if it builds (we do not use it for anything else), we want to run it at most once per build. I realized however that there is a scenario where we run ARM build twice (and it is not needed). You can see it here:
https://github.com/apache/airflow/runs/7205909316?check_suite_focus=true. This is direct push of `v2-3-test` branch:

(there was a failure of pushing cache to AMD which I have to look at but it's another issue)

Both "Build CI ARM Images 3.7...." and "Push Image Cache (linux/arm64)" perform an ARM image build. It has slighlty different build args in order to make the cache reusable for regular builds.

This "case" of build is not a regular "PR" run (`needs.build-info.outputs.merge-run == 'true'`). This is when we run the build AFTER we merged something to main or AFTER we pushed a cherry-picl to v2-3-test or AFTER we merged to 'v2-3-stable'. So this build does not "gate" anything. It's rather a regular "verification" build. What it really checks is "did dependencies change? shoudl we upgrade? Are we still passing all the tests and building image after they did?" - and it starts failing if for example someone releases a new dependency version that breaks our image build. it is the same "case" that is run after we merge to "main" build. This is "Push merge" as described here https://github.com/apache/airflow/blob/main/CI.rst#tests-workflow - this workflow description is not yet (I forgot it) updated to add the ARM image building but I will do it shortly and update the CI.rst (and move it to dev because it belongs there).

To summarize the important parts of the "push merge" build. Such builds run full combination of all python/backend versions and:

* attempts to "upgrade to newer dependencies" when building images - to see if there are some new dependencies
* if everything works it generates and push updated constraints with those dependencies
* using the new constraints it building cache in CI (so that subsequent "PR builds" do not have to update to the newer version of the dependencies).

What it also did (until this PR) is that it also run extra "Build ARM image" for verification. But since in this build we also run "Push Image Cache (linux/arm64)", there is no need to run the ARM image build separately. If the cache will fail, we will know it - by that time it's already too late, because someone released a dependency that broke our build and the only way we can react is to see the "main" build failing or see any of the "PRs" that update their own dependencies failing and we have to fix it.

So removing of the "ARM build" in this particular case (if needs.build-info.outputs.merge-run == 'true') does not change anything in PR builds.

--
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 #24858: Avoid running build ARM image twice

Posted by GitBox <gi...@apache.org>.

potiuk commented on PR #24858:
URL: https://github.com/apache/airflow/pull/24858#issuecomment-1176532743

   Does it sound good @jedcunningham :) ? 
   


-- 
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