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

Improve build performance through improving artifact patterns #317

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Feb 7, 2025

Performance: Optimize build time after hatchling migration as pointed out by @RRosio

Following our migration to hatchling as the build backend, build times nearly doubled. This PR implements several optimizations that significantly improve build performance:

  • Relocate individual files (non-globbed) from artifacts to force-include
  • Streamline glob patterns in artifacts

Performance improvements:

  • python -m build -w: 60s → 32s (47% reduction)

@danyeaw danyeaw force-pushed the optimize-artifact-list branch 3 times, most recently from 2df381c to 0355df2 Compare February 7, 2025 22:35
@RRosio RRosio added the enhancement New feature or request label Feb 10, 2025
@RRosio
Copy link
Collaborator

RRosio commented Feb 10, 2025

Hi @danyeaw, I tried running the Linux JS Tests locally and the first thing I noticed was that the failure in the CI (and replicated locally) was very similar to the error that I saw on main when I forgot to run npm run build. This leads wonder if the client-side JS files available once the tests are running.. I will take a closer look!

@danyeaw
Copy link
Contributor Author

danyeaw commented Feb 10, 2025

Hey @RRosio, do you have instructions for getting the dev environment set up to run the js tests? Thanks!

@RRosio
Copy link
Collaborator

RRosio commented Feb 10, 2025

Hey @RRosio, do you have instructions for getting the dev environment set up to run the js tests? Thanks!

Hi Dan, yes sorry I forgot to mention that, I followed the documentation here: https://github.com/jupyter/nbclassic/blob/b36454a787b4841531029741dbec93aa3be265b4/CONTRIBUTING.rst#javascript-tests

To verify the steps outlined above, I ran the tests on main, with the additional copying of the security_deprecated.js script over to base/js/security.js, they did pass. I thought that maybe the build step for notebook was not getting picked but when I looked into the files in the wheel and even in my source code, I can see the file is there.

@danyeaw danyeaw force-pushed the optimize-artifact-list branch from 0355df2 to 107f1f6 Compare February 11, 2025 21:10
@danyeaw danyeaw changed the title Improve build performance Improve build performance through improving artifact patterns Feb 11, 2025
@danyeaw
Copy link
Contributor Author

danyeaw commented Feb 11, 2025

The only test failure is a flaky test. @RRosio, I split off the change to use build.js to another PR. I think this one is ready 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants