Skip to content

Conversation

@vincbeck
Copy link
Contributor

Revert the revert #45057 of #44744.

It failed because of:

Traceback (most recent call last):
  File "/opt/airflow/./scripts/ci/pre_commit/compile_www_assets.py", line 86, in <module>
    if is_fab_provider_installed():
  File "/opt/airflow/./scripts/ci/pre_commit/compile_www_assets.py", line 79, in is_fab_provider_installed
    return importlib.util.find_spec("airflow.providers.fab") is not None
  File "/usr/local/lib/python3.9/importlib/util.py", line 94, in find_spec
    parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'airflow'

I updated the function is_fab_provider_installed and check if airflow is installed first. I ran breeze release-management prepare-airflow-package --package-format wheel and it succeeds (but fails without the fix).


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

@vincbeck vincbeck requested a review from jscheffl December 18, 2024 22:02
@vincbeck vincbeck requested review from ashb and potiuk as code owners December 18, 2024 22:02
@vincbeck vincbeck added the full tests needed We need to run full set of tests for this PR to merge label Dec 18, 2024
@vincbeck
Copy link
Contributor Author

Running all tests just to be sure

@vincbeck vincbeck closed this Dec 18, 2024
@vincbeck vincbeck reopened this Dec 18, 2024
@vincbeck vincbeck force-pushed the vincbeck/compile_assets branch from 3118f49 to 0fcbcfe Compare December 19, 2024 18:49
@vincbeck
Copy link
Contributor Author

vincbeck commented Feb 7, 2025

I updated this PR to create a new pre-commit to generate assets from FAB provider. It is basically the solution 2 described by @potiuk in this comment. I duplicated the script scripts/ci/pre_commit/compile_www_assets.py to scripts/ci/pre_commit/compile_fab_assets.pyto make it easier when we remove the Airflow 2 Flask UI, we will only have to delete scripts/ci/pre_commit/compile_www_assets.py (and its associated pre-commit).

The artifacts from FAB provider are not built on build time but are now part of the codebase.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit.

Looks good to me but we should wait for Jarek approval on this one.

@vincbeck
Copy link
Contributor Author

I dont quite understand why the static checks are failing 👀

@jedcunningham jedcunningham mentioned this pull request Feb 11, 2025
2 tasks
@vincbeck vincbeck requested review from potiuk and removed request for potiuk February 12, 2025 15:58
@vincbeck
Copy link
Contributor Author

vincbeck commented Feb 12, 2025

The static check seems to delete the generated files I checked in. I dont understand why, I am still trying to find out why

@potiuk
Copy link
Member

potiuk commented Feb 12, 2025

The static check seems to delete the generated files I checked in. I dont understand why, I am still trying to find out why

Without looking into details, it might be that generating the files (and pre-commit in general) that generates them has a slightly different environmen in CI than in your env - maybe the local env has different npm/yarn version (not pinned) or maybe there is another configuration setting in CI that makes the result different.

There are a number of "tricks" to make things reproducible regardless of the environment (for example when we generate image snapshot of breeze we fix the terminal width, and hard-code some variable things (like default number of processors) that can vary between the different systems.

Sometimes this is unavoidable - for example when things are run on Mac and Linux there might be some real differences - then we might revert to running stuff in docker (to ensure maximum environment similarity) or have another trick - often we can calculate hash of the "source" files (and we do it in a few places) and implement the pre-commit to only actually do something when the hash changes - we do that with breeze help snapshots. We store the hash as part of the commit and pre-commit will only actually regenerate stuff if the hash of sources changes. That might be an easy way out as well.

@vincbeck
Copy link
Contributor Author

The static check seems to delete the generated files I checked in. I dont understand why, I am still trying to find out why

Without looking into details, it might be that generating the files (and pre-commit in general) that generates them has a slightly different environmen in CI than in your env - maybe the local env has different npm/yarn version (not pinned) or maybe there is another configuration setting in CI that makes the result different.

There are a number of "tricks" to make things reproducible regardless of the environment (for example when we generate image snapshot of breeze we fix the terminal width, and hard-code some variable things (like default number of processors) that can vary between the different systems.

Sometimes this is unavoidable - for example when things are run on Mac and Linux there might be some real differences - then we might revert to running stuff in docker (to ensure maximum environment similarity) or have another trick - often we can calculate hash of the "source" files (and we do it in a few places) and implement the pre-commit to only actually do something when the hash changes - we do that with breeze help snapshots. We store the hash as part of the commit and pre-commit will only actually regenerate stuff if the hash of sources changes. That might be an easy way out as well.

Yep, that's what we do here. We generate a hash and re-generate the assets only if the hash changes. A per the output of the tests, the root cause might be because the CI fails to install dependencies with yarn. Looking at it :)

@jscheffl
Copy link
Contributor

The static check seems to delete the generated files I checked in. I dont understand why, I am still trying to find out why

Without looking into details, it might be that generating the files (and pre-commit in general) that generates them has a slightly different environmen in CI than in your env - maybe the local env has different npm/yarn version (not pinned) or maybe there is another configuration setting in CI that makes the result different.
There are a number of "tricks" to make things reproducible regardless of the environment (for example when we generate image snapshot of breeze we fix the terminal width, and hard-code some variable things (like default number of processors) that can vary between the different systems.
Sometimes this is unavoidable - for example when things are run on Mac and Linux there might be some real differences - then we might revert to running stuff in docker (to ensure maximum environment similarity) or have another trick - often we can calculate hash of the "source" files (and we do it in a few places) and implement the pre-commit to only actually do something when the hash changes - we do that with breeze help snapshots. We store the hash as part of the commit and pre-commit will only actually regenerate stuff if the hash of sources changes. That might be an easy way out as well.

Yep, that's what we do here. We generate a hash and re-generate the assets only if the hash changes. A per the output of the tests, the root cause might be because the CI fails to install dependencies with yarn. Looking at it :)

Mhm, Edge with the legacy UI probably will follow soon also by adding some UIs to replace the legacy. I assume there might be more coming step by step. I'd favor a dynamic generation at packaging time but I am also OK if we make it static first and then change later.

@vincbeck vincbeck force-pushed the vincbeck/compile_assets branch from e56a5fa to 4756a08 Compare February 12, 2025 20:00
@vincbeck vincbeck force-pushed the vincbeck/compile_assets branch from 4756a08 to 3bebbc8 Compare February 12, 2025 20:38
@vincbeck vincbeck merged commit 7323548 into apache:main Feb 13, 2025
91 checks passed
@vincbeck vincbeck deleted the vincbeck/compile_assets branch February 13, 2025 16:20
@vincbeck
Copy link
Contributor Author

Finally!

jedcunningham added a commit to astronomer/airflow that referenced this pull request Feb 13, 2025
One of the compiled assets from apache#45060 is failing static checks in main.
vincbeck pushed a commit that referenced this pull request Feb 13, 2025
One of the compiled assets from #45060 is failing static checks in main.
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
One of the compiled assets from apache#45060 is failing static checks in main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants