Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resume yandex provider #33574

Merged
merged 4 commits into from
Aug 22, 2023
Merged

Resume yandex provider #33574

merged 4 commits into from
Aug 22, 2023

Conversation

s0neq
Copy link
Contributor

@s0neq s0neq commented Aug 21, 2023

Yandex Provider was suspended in #30667
This PR is for resuming Yandex Provider in Airflow releases


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@s0neq s0neq requested a review from potiuk as a code owner August 21, 2023 08:40
@potiuk potiuk changed the title resume yandex provider. new 'breeze setup regenerate-command-images' … Resume yandex provider Aug 21, 2023
@potiuk
Copy link
Member

potiuk commented Aug 21, 2023

Oh. Welcome back yandex provider ... Indeed I see that protobuf dependency has been bumped. Let's see where it gets us :)

@potiuk
Copy link
Member

potiuk commented Aug 21, 2023

Oh. Welcome back yandex provider ... Indeed I see that protobuf dependency has been bumped. Let's see where it gets us :)

Breeze tests needs to be updated as well.

@Piatachock
Copy link
Contributor

Piatachock commented Aug 21, 2023

LGTM from Yandex
Also, hi Jarek. :)

@s0neq
Copy link
Contributor Author

s0neq commented Aug 21, 2023

hi, Jarek, hi Peter!

there are some strange problems in tests, should I close and reopen the Pull Request?

from Helm: CeleryExecutor - true - v1.24.15 v1.25.11 v1.26.6 v1.27.3

We are extremely sorry but you've hit the rare case that the credentials you got from GitHub Actions to run are expired, and we cannot do much.
¯_(ツ)_/¯

You have the following options now:

  • Close and reopen the Pull Request of yours
  • Rebase or amend your commit and push your branch again
  • Ask in the PR to re-run the failed job

@potiuk
Copy link
Member

potiuk commented Aug 21, 2023

1). There is a "build-docs" failure to fix (the links to changelog and security pages were moved and yandex doc is still pointing to the old location

  1. there is another issue with pip backtracking but I will take a look at it after the 1) is fixed

The other issues are just "rebuild" needed - but when you fix the docs and push, the build will re-run for all of them

@s0neq
Copy link
Contributor Author

s0neq commented Aug 21, 2023

@potiuk build-docs is fixed, please take a look at the second issue

potiuk added a commit to potiuk/airflow that referenced this pull request Aug 22, 2023
Previously we have been using provider.yaml file modification as
a sign that the docker image needs rebuilding when starting image.
However just modification of provider.yaml file is not a sign
that the image needs rebuilding. The image needs rebuilding when
provider dependencies changed, but there are many more reasons why
provider.yaml file changed - especially recently provider.yaml
file contains much more information and dependencies are only part
of it. Provider.yaml files can also be modified by release manager
wnen documentation is prepared, but none of the documentation
change is a reason for rebuilding the image.

This PR optimize the check for image building introducing two
step process:

* first we check if provider.yaml files changed
* if they did, we regenerate provider dependencies by manully
  running the pre-commit script
* then provider_dependencies.json is used instead of all providers
  to determine if the image needs rebuilding

This has several nice side effects:

* the list of files that have been modified displayed to the
  user is potentially much smaller (no provider.yaml files)
* provider_dependencies.json is regenereated automatically when
  you run any breeze command, which means that you do not have
  to have pre-commit installed to regenerate it
* the notification "image needs rebuilding" will be printed less
  frequently to the user - only when it is really needed
* preparing provider documentation in CI will not trigger
  image rebuilding (which might occasionally fail in such case
  especially when we bring back a provider from long suspension
  like it happened in apache#33574
@potiuk
Copy link
Member

potiuk commented Aug 22, 2023

@potiuk build-docs is fixed, please take a look at the second issue

Interesting one. It actually revealed an interesting bug/edge case/optimization possibility.

And it motivated me to implement #33603 - optimization that I wanted to do for some time.

What happened in this case - the job that failed was run as part of provider check and in this check we do two things:

  1. we simulate preparation of provider decumentation - as if release manager run the documentation generation (helps us to check if the tooling still works)
  2. then we build provider packages

The point 1) modified provider.yaml files for all the providers that were updated since last release (including yandex)

Then, when 2) was attempted, breeze detected that provider.yaml files changed and attempted to rebuild the CI image - normally it is a "no-op" because we just built the image with the constraints. But yandex provider has no constraints generated, so pip attempted to build the image without constraints, and apparently some of the constraints of yandes are not compatible with the current set of constraints so building the image faild.

The optimization in #33603 will solve it in a bit indirect way - when provider.yaml are updated by documentation preparation, breeze will first attempt to regenerate "generated/provider_dependencies.json" and only this file change it will attempt to rebuild the image.

Result - slightly faster and more accurate "check providers" job in CI, less frequent prompts for rebuilding the image for regular developers in breeze, the "generated/provider_dependencies.json" will be generated as needed when you just run breeze, without the need to have pre-commit installed.

Once #33603 gets merged. you should rebase and I believe the job will succeed.

potiuk added a commit that referenced this pull request Aug 22, 2023
* Improve detection of when breeze CI image needs rebuilding

Previously we have been using provider.yaml file modification as
a sign that the docker image needs rebuilding when starting image.
However just modification of provider.yaml file is not a sign
that the image needs rebuilding. The image needs rebuilding when
provider dependencies changed, but there are many more reasons why
provider.yaml file changed - especially recently provider.yaml
file contains much more information and dependencies are only part
of it. Provider.yaml files can also be modified by release manager
wnen documentation is prepared, but none of the documentation
change is a reason for rebuilding the image.

This PR optimize the check for image building introducing two
step process:

* first we check if provider.yaml files changed
* if they did, we regenerate provider dependencies by manully
  running the pre-commit script
* then provider_dependencies.json is used instead of all providers
  to determine if the image needs rebuilding

This has several nice side effects:

* the list of files that have been modified displayed to the
  user is potentially much smaller (no provider.yaml files)
* provider_dependencies.json is regenereated automatically when
  you run any breeze command, which means that you do not have
  to have pre-commit installed to regenerate it
* the notification "image needs rebuilding" will be printed less
  frequently to the user - only when it is really needed
* preparing provider documentation in CI will not trigger
  image rebuilding (which might occasionally fail in such case
  especially when we bring back a provider from long suspension
  like it happened in #33574

* Update dev/breeze/src/airflow_breeze/commands/developer_commands.py
@potiuk
Copy link
Member

potiuk commented Aug 22, 2023

OK. Merged my change. Now rebased to see if it helps

@potiuk
Copy link
Member

potiuk commented Aug 22, 2023

The ARM image failure is irrelevant. Merging.

@potiuk potiuk merged commit 2b43fa4 into apache:main Aug 22, 2023
62 of 64 checks passed
@potiuk
Copy link
Member

potiuk commented Aug 22, 2023

AAAND merged !

@s0neq
Copy link
Contributor Author

s0neq commented Aug 23, 2023

AAAND merged !

yayy thanks!

@s0neq
Copy link
Contributor Author

s0neq commented Aug 23, 2023

@potiuk hello again! just small question - when do you think these changes will be released?

@potiuk
Copy link
Member

potiuk commented Aug 23, 2023

Question to @eladkal but we usually release providers every 2-3 weeks. We also would likely want to release providers before 2.7.1 is out (~ week or so). So generally speaking sooner rather than later

@eladkal
Copy link
Contributor

eladkal commented Aug 23, 2023

I'll do my best to get to it soon

potiuk added a commit that referenced this pull request Aug 25, 2023
* Improve detection of when breeze CI image needs rebuilding

Previously we have been using provider.yaml file modification as
a sign that the docker image needs rebuilding when starting image.
However just modification of provider.yaml file is not a sign
that the image needs rebuilding. The image needs rebuilding when
provider dependencies changed, but there are many more reasons why
provider.yaml file changed - especially recently provider.yaml
file contains much more information and dependencies are only part
of it. Provider.yaml files can also be modified by release manager
wnen documentation is prepared, but none of the documentation
change is a reason for rebuilding the image.

This PR optimize the check for image building introducing two
step process:

* first we check if provider.yaml files changed
* if they did, we regenerate provider dependencies by manully
  running the pre-commit script
* then provider_dependencies.json is used instead of all providers
  to determine if the image needs rebuilding

This has several nice side effects:

* the list of files that have been modified displayed to the
  user is potentially much smaller (no provider.yaml files)
* provider_dependencies.json is regenereated automatically when
  you run any breeze command, which means that you do not have
  to have pre-commit installed to regenerate it
* the notification "image needs rebuilding" will be printed less
  frequently to the user - only when it is really needed
* preparing provider documentation in CI will not trigger
  image rebuilding (which might occasionally fail in such case
  especially when we bring back a provider from long suspension
  like it happened in #33574

* Update dev/breeze/src/airflow_breeze/commands/developer_commands.py

(cherry picked from commit ac0d5b3)
@ephraimbuddy ephraimbuddy modified the milestone: Airflow 2.7.1 Sep 1, 2023
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation provider:yandex
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants