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

Generate sync methods from async ones #1329

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

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Feb 23, 2025

This PR is built on top of #1328.

In addition to that PR, it provides a synchronous version of build_sync_jupytext_contents_manager_class that is extracted programmatically from the asynchronous version.

Copy link

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/mwouts/jupytext.git@async_and_sync_in_sync

(this requires nodejs, see more at Developing Jupytext)

@mwouts mwouts force-pushed the async_and_sync_in_sync branch 2 times, most recently from c48fdf3 to 0c0f754 Compare February 23, 2025 23:27
@mwouts mwouts force-pushed the async_and_sync_in_sync branch from 0c0f754 to 093bfb9 Compare February 23, 2025 23:31
@mwouts mwouts marked this pull request as ready for review February 23, 2025 23:33
Copy link

codecov bot commented Feb 23, 2025

Codecov Report

Attention: Patch coverage is 90.95023% with 40 lines in your changes missing coverage. Please review.

Project coverage is 96.45%. Comparing base (9aba222) to head (093bfb9).

Files with missing lines Patch % Lines
src/jupytext/async_contentsmanager.py 87.98% 40 Missing ⚠️

❌ Your project check has failed because the head coverage (96.40%) is below the target coverage (97.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1329      +/-   ##
==========================================
- Coverage   97.04%   96.45%   -0.60%     
==========================================
  Files          29       32       +3     
  Lines        4499     4876     +377     
==========================================
+ Hits         4366     4703     +337     
- Misses        133      173      +40     
Flag Coverage Δ
external 70.19% <24.20%> (-4.88%) ⬇️
functional ?
integration 78.18% <90.27%> (+0.89%) ⬆️
unit 62.32% <22.62%> (-4.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwouts
Copy link
Owner Author

mwouts commented Feb 23, 2025

Hi @Darshan808 , I see more tests passing on this PR. In this version, the synchronous contents manager is really synchronous, which means that we don't need to adjust the remaining tests. Can you have a look and let me know what you think? Now that we see at least one way to get the tests passing I will have a look at the next step i.e. test this in real conditions over the coming days.

@Darshan808
Copy link

Can you have a look and let me know what you think?

I initially avoided this approach because of code duplication and the hassle of maintaining the same code in two places. But the idea of a test duplicating the file is awesome—it solves that issue. Both approaches have trade-offs, but this seems fine. I think we can proceed!

@Darshan808
Copy link

Looking at the CI failure, I think adding black as a dependency to the functional test should fix it

@mwouts
Copy link
Owner Author

mwouts commented Feb 24, 2025

Thanks! I'll do another pass in the coming days. One more question - do you think we will ever want to create an async contents manager from a sync one? If not maybe we can remove the ensure_async calls in the contents manager builder?

@Darshan808
Copy link

do you think we will ever want to create an async contents manager from a sync one?

After separating sync and async, this scenario should no longer occur. I believe we can safely remove ensure_async for method calls.

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

Successfully merging this pull request may close these issues.

2 participants