mwouts / jupytext

Jupyter Notebooks as Markdown Documents, Julia, Python or R scripts
https://jupytext.readthedocs.io
MIT License
6.65k stars 386 forks source link

Check `ContentsManager` base class using `inspect` #1254

Closed mahendrapaipuri closed 4 months ago

mahendrapaipuri commented 4 months ago

Closes #1239

@mwouts What do you think of this change? I have bumped jupytext version in tests to use new SyncMetaManager

github-actions[bot] commented 4 months ago

Thank you for making this pull request.

Did you know? You can try it on Binder: Binder:lab or Binder:notebook.

Also, the version of Jupytext developed in this PR can be installed with pip:

HATCH_BUILD_HOOKS_ENABLE=true pip install git+https://github.com/mahendrapaipuri/jupytext.git@check_contents_mgr_type

(this requires nodejs, see more at Developing Jupytext)

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.04%. Comparing base (29a979f) to head (303c8fb). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1254 +/- ## ========================================== - Coverage 97.76% 97.04% -0.72% ========================================== Files 29 29 Lines 4468 4468 ========================================== - Hits 4368 4336 -32 - Misses 100 132 +32 ``` | [Flag](https://app.codecov.io/gh/mwouts/jupytext/pull/1254/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts) | Coverage Δ | | |---|---|---| | [external](https://app.codecov.io/gh/mwouts/jupytext/pull/1254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts) | `?` | | | [functional](https://app.codecov.io/gh/mwouts/jupytext/pull/1254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts) | `?` | | | [integration](https://app.codecov.io/gh/mwouts/jupytext/pull/1254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts) | `?` | | | [unit](https://app.codecov.io/gh/mwouts/jupytext/pull/1254/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Marc+Wouts#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mwouts commented 4 months ago

Hi @mahendrapaipuri , thank you for the PR. Yes that sounds great! And it's exactly the test that I wanted, thanks.

Maybe we could also say a word about how to use Jupytext with jupyter-fs==1.0 in the documentation (I mean, the test is great but not everyone will find it).

I'm not sure what happened to the CI? In a recent run it was green but I have seen a few varying failures recently.

mahendrapaipuri commented 4 months ago

Maybe we could also say a word about how to use Jupytext with jupyter-fs==1.0 in the documentation (I mean, the test is great but not everyone will find it).

Yes, I agree. I forgot about the docs part. I will update the PR with a word about it.

I'm not sure what happened to the CI? In a recent run it was green but I have seen a few varying failures recently.

I think the failure is due to a bug in jupytext==1.0.0 that has been fixed recently. But there was no new release with this bugfix. Should we revert to jupytext==0.4.0 in the tests?

mwouts commented 4 months ago

Maybe we could also say a word about how to use Jupytext with jupyter-fs==1.0 in the documentation (I mean, the test is great but not everyone will find it).

Yes, I agree. I forgot about the docs part. I will update the PR with a word about it.

Thanks Mahendra. I can also take care of that over the week-end.

I think the failure is due to a bug in jupytext==1.0.0 that has been fixed recently. But there was no new release with this bugfix. Should we revert to jupytext==0.4.0 in the tests?

You mean in jupyter-fs? It looks like the CI fails on the lab extension test, and in the logs there is indeed something about jupyter-fs. Maybe installing jupyter-fs changes the default contents manager now?

mahendrapaipuri commented 4 months ago

You mean in jupyter-fs? It looks like the CI fails on the lab extension test, and in the logs there is indeed something about jupyter-fs. Maybe installing jupyter-fs changes the default contents manager now?

Yes, indeed I meant jupyter-fs. No, it is not about the contents manager. I think it is more due to an error in UI due to the bug in jupyter-fs that has been fixed. I will try to look into it in more detail

mahendrapaipuri commented 4 months ago

So, when we use jupyter-fs, we need to set ContentsManager class to the one shipped by jupyter-fs and if it is not configured, the extension will not be registered.

Moreoever, jupyter-fs is overriding the browser-test shipped by jupyter-lab to test its own functionality. As in the lab extension test in CI, we are not setting ContentsManager to the one from the jupyter-fs, the extension is not being activated and hence browser test fails.

I guess we should not "test" functionality of jupyter-fs extension in jupytext CI. I propose that we move lab extension test as the last step in workflow and uninstall jupyter-fs before lab extension test so that we wont have these issues.

@mwouts What do you think? Btw, there is a PR to bump jupyter-fs version with bug fixes. Maybe we can wait until they cut a new release?

mwouts commented 4 months ago

Hi Mahendra, thank you for looking into this!

I agree with your suggestion to uninstall jupyter-fs before running the UI tests.

Do we need the next jupyter-fs release to make the tests pass? If we don't I would rather take your PR sooner, as I am thinking of publishing a new release.

mahendrapaipuri commented 4 months ago

@mwouts No, we dont need to wait for the release for the tests to pass. I added a comment to keep track of it in pyproject.toml and pushed new changes. Let me know what do you think!

mwouts commented 4 months ago

Thank you @mahendrapaipuri ! That looks great. I will fix the issue with the CI, take your PR (I might relax the requirement on jupyterfs in tests from ==1.0.0 to >=1.0 if that's fine with you) and publish a new release asap.

mahendrapaipuri commented 4 months ago

I might relax the requirement on jupyterfs in tests from ==1.0.0 to >=1.0 if that's fine with you

That was a good idea. Cheers!