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

Move javascript compilation to host #25169

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 19, 2022

Previously, in order to keep consistent development environment
we've compiled javascript in the CI image. However we can utilise
power of pre-commmit for setting the node environment for all
contributors automatically. Instead of compiling the javascript
in the image, we can compile it via pre-commit in the host.

This can be done thanks to the new python breeze which is far more
flexible and can now add execution of compilation of the javascript when
needed and using pre-commit environments.

Thanks to that, we can vastly simplify the Dockerfiles and scripts that
are used to automatically build or signal that the assets need
recompilation. We can basically assume that the assets were prepared
outside of the image building (and breeze makes sure it happens)

The changes:

  • node.js is not needed in images (neither PROD build nor CI)
  • no need for multiple asset compilation scripts. All is done
    via pre-commit environment with `breeze compile-www-assets``
    command
  • lint checks for UI do not need the docker image any more
    (they are also based on pre-commit environment)
  • no more checks/warnings when you enter the image
  • start-airflow command builds the compilation before entering
  • prepare-airflow-package runs asset compilation before entering
    docker airflow building.

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

@potiuk
Copy link
Member Author

potiuk commented Jul 19, 2022

cc: @pierrejeambrun @bbovenzi - please take a look, I think you will find this change really cool.

@potiuk potiuk force-pushed the simplify-www-asset-compilation branch from e1a2212 to 225c1e8 Compare July 19, 2022 22:28
@potiuk potiuk requested a review from kaxil as a code owner July 19, 2022 22:28
@potiuk potiuk force-pushed the simplify-www-asset-compilation branch 4 times, most recently from 34fe38c to dc46ffa Compare July 20, 2022 06:34
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

Tested this out locally. Everything seems to be working. But does look like we need a quick rebase.

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Cool. Yeah. I will . :)

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Thanks for testing @bbovenzi - > I think this will help a bit with faster iterations and maybe we could also add some more dev-friendly javascript tooling around to attract more contributors and make it easier to start/test/debug Javascript code. Especially with the typescript and React, I have a feeling we could add some tooling to breeze to improve "first-time-experience" for new users. Happy to brainstorm on it and implement any improvements there :)

@potiuk potiuk force-pushed the simplify-www-asset-compilation branch from dc46ffa to 1a949e4 Compare July 20, 2022 12:42
@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Hm. Interestingly enough - we seem to have not enough space on our machines to get THREE yarn installs :)

@bbovenzi
Copy link
Contributor

Hm. Interestingly enough - we seem to have not enough space on our machines to get THREE yarn installs :)

Are we not clearing the cache each time?

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Not when we have three independent yarn builds run in pre-commits - the envs are created in ~/.cache which is rather small it seems, But I am looking into it.

@potiuk potiuk force-pushed the simplify-www-asset-compilation branch from 1a949e4 to 1ffd5a1 Compare July 20, 2022 15:00
@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Let's see now :)

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Added more disk space for the instance (later I can optimize it even more).

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Now we are cooking with gas. I will just re-run it with public runners to be absolutely sure the yarn modules will not make it run out of the capacity either.

@potiuk potiuk added the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Jul 20, 2022
Previously, in order to keep consistent development environment
we've compiled javascript in the CI image. However we can utilise
power of pre-commmit for setting the node environment for all
contributors automatically. Instead of compiling the javascript
in the image, we can compile it via pre-commit in the host.

This can be done thanks to the new python breeze which is far more
flexible and can now add execution of compilation of the javascript when
needed and using pre-commit environments.

Thanks to that, we can vastly simplify the Dockerfiles and scripts that
are used to automatically build or signal that the assets need
recompilation. We can basically assume that the assets were prepared
outside of the image building (and breeze makes sure it happens)

The changes:

* node.js is not needed in images (neither PROD build nor CI)
* no need for multiple asset compilation scripts. All is done
  via pre-commit environment with `breeze compile-www-assets``
  command
* lint checks for UI do not need the docker image any more
  (they are also based on pre-commit environment)
* no more checks/warnings when you enter the image
* start-airflow command builds the compilation before entering
* prepare-airflow-package runs asset compilation before entering
  docker airflow building.
@potiuk potiuk force-pushed the simplify-www-asset-compilation branch from 1ffd5a1 to f07c3d8 Compare July 20, 2022 16:12
@potiuk potiuk merged commit acff129 into main Jul 20, 2022
@MatrixManAtYrService
Copy link
Contributor

I have some automation which runs python setup.py compile_assets bdist_wheel in service of creating an airflow image for test. Following this commit, it fails with:

[Errno 2] No such file or directory: './airflow/www/compile_assets.sh': './airflow/www/compile_assets.sh'

Is that a bug, or is there a better way for me to be building this?

@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

A bug. fix is coming

potiuk added a commit to potiuk/airflow that referenced this pull request Jul 20, 2022
Asset compilation via setup.py has been broken in apache#25169.

This PR fixes it.
@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

PR here: #25201

potiuk added a commit that referenced this pull request Jul 20, 2022
Asset compilation via setup.py has been broken in #25169.

This PR fixes it.
@potiuk
Copy link
Member Author

potiuk commented Jul 20, 2022

Merged. Should be fixed now @MatrixManAtYrService

potiuk added a commit that referenced this pull request Jul 21, 2022
Previously, in order to keep consistent development environment
we've compiled javascript in the CI image. However we can utilise
power of pre-commmit for setting the node environment for all
contributors automatically. Instead of compiling the javascript
in the image, we can compile it via pre-commit in the host.

This can be done thanks to the new python breeze which is far more
flexible and can now add execution of compilation of the javascript when
needed and using pre-commit environments.

Thanks to that, we can vastly simplify the Dockerfiles and scripts that
are used to automatically build or signal that the assets need
recompilation. We can basically assume that the assets were prepared
outside of the image building (and breeze makes sure it happens)

The changes:

* node.js is not needed in images (neither PROD build nor CI)
* no need for multiple asset compilation scripts. All is done
  via pre-commit environment with `breeze compile-www-assets``
  command
* lint checks for UI do not need the docker image any more
  (they are also based on pre-commit environment)
* no more checks/warnings when you enter the image
* start-airflow command builds the compilation before entering
* prepare-airflow-package runs asset compilation before entering
  docker airflow building.

(cherry picked from commit acff129)
potiuk added a commit that referenced this pull request Jul 21, 2022
Asset compilation via setup.py has been broken in #25169.

This PR fixes it.

(cherry picked from commit c3763f3)
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 25, 2022
When we moved www asset compilation from container to the host in apache#25169,
we removed yarn from the image and the old instructions are no longer
valid. Instead of in-container yarn dev we expect now
to have yarn dev run in the host. It can be done either with
pre-commit (if you have no node/yarn installed in your host, pre-commit
will automatically install both node and yarn in the right versions,
or if you have yarn installed you can run it manually.

The 'start-airflow' instructions now remove the in-container
instructions and explain those two options you have.
potiuk added a commit that referenced this pull request Jul 25, 2022
When we moved www asset compilation from container to the host in #25169,
we removed yarn from the image and the old instructions are no longer
valid. Instead of in-container yarn dev we expect now
to have yarn dev run in the host. It can be done either with
pre-commit (if you have no node/yarn installed in your host, pre-commit
will automatically install both node and yarn in the right versions,
or if you have yarn installed you can run it manually.

The 'start-airflow' instructions now remove the in-container
instructions and explain those two options you have.
potiuk added a commit that referenced this pull request Aug 4, 2022
When we moved www asset compilation from container to the host in #25169,
we removed yarn from the image and the old instructions are no longer
valid. Instead of in-container yarn dev we expect now
to have yarn dev run in the host. It can be done either with
pre-commit (if you have no node/yarn installed in your host, pre-commit
will automatically install both node and yarn in the right versions,
or if you have yarn installed you can run it manually.

The 'start-airflow' instructions now remove the in-container
instructions and explain those two options you have.

(cherry picked from commit 1544868)
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 14, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.4 milestone Aug 15, 2022
@potiuk potiuk deleted the simplify-www-asset-compilation branch May 6, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:production-image Production image improvements and fixes area:webserver Webserver related Issues kind:documentation type:misc/internal Changelog: Misc changes that should appear in change log use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants