You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2024/02/10 23:07:10 UTC
[PR] Add better diagnostics when provider.yaml check fails [airflow]
potiuk opened a new pull request, #37322:
URL: https://github.com/apache/airflow/pull/37322
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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+Improvement+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` or `{issue_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
Re: [PR] Add better diagnostics when provider.yaml check fails [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #37322:
URL: https://github.com/apache/airflow/pull/37322#issuecomment-1937400129
> And adding new providers/modifying dependencies is generallly very rare event that we should not optimize for.
This makes sense and I agree.
And, Thank you so much for such a detailed response.
--
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
Re: [PR] Add better diagnostics when provider.yaml check fails [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37322:
URL: https://github.com/apache/airflow/pull/37322#issuecomment-1937397915
> @potiuk , Can you please confirm if this check only runs when there is a change in the provider.yaml file?
Also it's not the case. In CI we always run "all" pre-commits. Pre-comit has two modes, one local (when it only run pre-commits that are related to locally modified files), and one usually used in ci where `--all-files` switch is used.
In CI in case dependencies were modified, we run `--upgrade-to-newer-dependencies`, but even there as of https://github.com/apache/airflow/pull/37305 merged few hours ago, we run `--upgrade-to-newer-dependencies` not when provider.yaml change, but when we detect that the resulting `pyproject.toml` has changed dependencies.
And again - this is because running `--upgrade-to-newer-dependencies` with ~700 dependencies takes a lot of time (27 minutes in CI), not only because there is a lot of data, to pull but also because `pip` performs resolution of those dependencies and tries to find the newest versions of those dependencies. So `--upgrade-to-newer-deps` should be the last resort and deliberately run when you really need 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add better diagnostics when provider.yaml check fails [airflow]
Posted by "rawwar (via GitHub)" <gi...@apache.org>.
rawwar commented on PR #37322:
URL: https://github.com/apache/airflow/pull/37322#issuecomment-1937383896
@potiuk , Can you please confirm if this check only runs when there is a change in the `provider.yaml` file?
If so, can we try to actually pass `--upgrade-to-newer-dependencies` argument to the breeze directly in the pre-commit? If there are package upgrades, they will upgrade.
If not, would adding this help? I can look into this.
--
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
Re: [PR] Add better diagnostics when provider.yaml check fails [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #37322:
URL: https://github.com/apache/airflow/pull/37322#issuecomment-1937396102
> If so, can we try to actually pass `--upgrade-to-newer-dependencies` argument to the breeze directly in the pre-commit? If there are package upgrades, they will upgrade.
No - we do not want to do it automatically, because running it takes a long time and it takes a lot of time and networking, so you should not really run it automatically when provider.yaml changes, because the fact that it changes, does not mean that you HAVE TO do it. In many situations, running pre-commits will be fine even if you don't run it, and it's fine, because when you have slow network, develop while in a plane etc, you don't want to pull 700 MB of data.
The whole image rebuilding is designed around it - it should be "eventually consistent" - you are gently guided to rebuild the image, but you should not be forced to run the rebuild, but not forced, because you might not want to do it **just now**.
And adding new providers/modifying dependencies is generallly very rare event that we should not optimize for. What you experience might seam common, but there is a very low percentage of PRs that add or modify dependencies or change provider.yaml file (and even less that result in absolute need or rebuilding the image).
This is explained in detail in the two ADRs (Architecture Decision Records): [ADR4](https://github.com/apache/airflow/blob/main/dev/breeze/doc/adr/0004-using-docker-images-as-test-environment.md) where it is explained why we chose docker container as "common" environment and [ADR12](https://github.com/apache/airflow/blob/main/dev/breeze/doc/adr/0012-asking-user-for-confirmation.md) - where the mechanism of eventual consistency and asking the user for confirmation is explained.
--
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
Re: [PR] Add better diagnostics when provider.yaml check fails [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #37322:
URL: https://github.com/apache/airflow/pull/37322
--
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