Don't build the extension in pre-commit environments #1226
Conversation
|
Thank you for making this pull request. Did you know? You can try it on Binder: Also, the version of Jupytext developed in this PR can be installed with (this requires |
mwouts
force-pushed
the
no_extension_in_pre_commit_envs
branch
from
b804afb
to
33b9496
Compare
|
Apparently I have been able to require |
Well apparently that did not work 😢 (the dev install fails on the CI) |
mwouts
force-pushed
the
no_extension_in_pre_commit_envs
branch
from
33b9496
to
c607e7d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1226 +/- ##
=======================================
Coverage 97.73% 97.73%
=======================================
Files 29 29
Lines 4463 4463
=======================================
Hits 4362 4362
Misses 101 101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| """ | ||
|
|
||
| def initialize(self, version, _): | ||
| if "/.cache/pre-commit/".replace("/", os.path.sep) in sys.executable: |
There was a problem hiding this comment.
This is a good idea. But I am not quite sure how are you detecting if the current installation is done by pre-commit and not a regular installation.
There was a problem hiding this comment.
Well, so far I have not found a better way than checking whether the Python path contains /.cache/pre-commit/ (I still need to check how it looks like on Windows)
| @@ -130,14 +130,14 @@ packages = ["src/jupytext", "src/jupytext_config", "jupyterlab/jupyterlab_jupyte | |||
| "jupyterlab/jupyter-config" = "etc/jupyter" | |||
| "jupyterlab/jupyterlab_jupytext/labextension" = "share/jupyter/labextensions/jupyterlab-jupytext" | |||
|
|
|||
| [tool.hatch.build.hooks.jupyter-builder] | |||
| [tool.hatch.build.hooks.custom] | |||
| # Enable running hook by default. | |||
| # We disable this hook only by setting env var HATCH_BUILD_HOOKS_ENABLE=false | |||
| # So `HATCH_BUILD_HOOKS_ENABLE=false pip install .` will **not** build JupyterLab related | |||
| # extension. A simple `pip install .` will **always** build extension | |||
| enable-by-default = true | |||
There was a problem hiding this comment.
Why not set enable-by-default = false, so that extension will not build by default. In the CI, we will need to add HATCH_BUILD_HOOKS_ENABLE=true to force the building of extension.
There was a problem hiding this comment.
Well sure that would solve this issue indeed! That's how we were doing it up to version 1.14.x I believe. Would that be ok with you? I'll be careful to make sure that we don't miss the extension in the conda package too.
There was a problem hiding this comment.
From my understanding, most of the jupytext "contributors" are interested in developing core jupytext and do not touch its extension. If that is the case, I think it makes more sense to disable extension building by default as it completely avoids node dependency.
So, yes, I think it makes sense for me to set enable-by-default = false and more over, that is what I proposed originally when we moved to hatch build system.
|
As discussed above, I have merged #1233 instead - thanks @mahendrapaipuri for your advice! |
With this PR, the extension is not build when the Python executable looks like a pre-commit environment.
Closes #1210
After this PR, the test
tests/external/pre_commit/test_pre_commit_1_sync_with_no_config.pyruns in 25 seconds (was 54 seconds previously).At least this should fix the issue that Jupytext can't be installed in a pre-commit hook env when npm is not present.
If I new how to do that, I would prefer to not even take a dependency on
hatch-jupyter-builderandjupyterlab(I think that's where most of the 25 seconds come from).I see that it is possible to have dynamical values in the
projecttable (e.g.version, or evendependencies, so I have been thinking of writing a hook that would identify pre-commit environments and set e.g. abuild_for_pre_commit_envvariable, but then@mahendrapaipuri do you think I could use that variable to skip the extension build?