Add async contents manager support

[Darshan808]

Fixes #1316

This PR introduces support for an async contents manager. As suggested by @mwouts in #1316, a separate function, build_jupytext_async_contents_manager, has been implemented in a new file, async_contentsmanager.py.

To ensure full test coverage, the existing tests have been duplicated into test_async_contentsmanager.py. Currently, all tests are passing! ✅

[github-actions]

Thank you for making this pull request.

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

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/Darshan808/jupytext.git@async_support

(this requires nodejs, see more at Developing Jupytext)

[Darshan808]

@mwouts
Could you please review this? I’d appreciate your feedback before proceeding further.

As we begin merging these two files, should we wrap all asynchronous functions (that might be synchronous in the synchronous contents manager) using ensure_async(), similar to what was done in #1021?

Looking forward to your thoughts!

[mwouts]

Fixes #1316

This PR introduces support for an async contents manager. As suggested by @mwouts in #1316, a separate function, build_jupytext_async_contents_manager, has been implemented in a new file, async_contentsmanager.py.

To ensure full test coverage, the existing tests have been duplicated into test_async_contentsmanager.py. Currently, all tests are passing! ✅

Wow, thank you @Darshan808 , this is truly amazing!! Let me have a look!

[mwouts]

Thank you @Darshan808 , I love what I see so far!

As we begin merging these two files, should we wrap all asynchronous functions (that might be synchronous in the synchronous contents manager) using ensure_async(), similar to what was done in #1021?

Do you think it would be possible to do so? It would be great indeed if we had a more local change - easier to review, and just one file to maintain later on. But I must admit that it's precisely what I was not able to do two years ago so no obligation at all!

[mwouts]

Yes that's the most prudent approach, thank you! The import problem was years ago though, so maybe it's gone, but it's safer that I follow-up on that later on.

[mwouts]

Yes exactly what I had in mind! And we can delete the paragraph above.

[mwouts]

Re the failing pre-commit hook on the CI you probably just need pre-commit install - see also https://jupytext.readthedocs.io/en/latest/developing.html#install-and-develop-jupytext-locally

[Darshan808]

@mwouts
I merged the two files, wrapping methods with ensure_async(), and merged the tests as well. To support both synchronous and asynchronous ContentsManager, I added a pytest fixture that provides both managers to all tests. All tests are passing successfully.
Let me know what you think about this change!

[Darshan808]

Locally I checked only test_contentsmanager.py, but it looks like other tests are affected as well. I'll look into it.

[mwouts]

Hi @Darshan808 , thanks for the update, and no worries for the remaining failures, that's a big change you know! Also I hope you're fine with me pushing a few commits here.

One thing that we need to confirm is the following: are we fine with build_jupytext_contents_manager_class always returning an async contents manager?

Async contents manager are a bit more complicated to use in tests, so that could be an incentive to provide a sync version, but I think that it has been a while since async contents manager were introduced so it's probably fine to have just them.

In case we wanted both, do you think we could write a second function e.g. build_sync_contents_manager_class(async_base_class) to convert the async cm to a sync one? Do you see any other way to avoid duplicating the code?

[mwouts]

Locally I checked only test_contentsmanager.py, but it looks like other tests are affected as well. I'll look into it.

Yes - I think that relates to my comment above, now every contents manager class returned by build_jupytext_contents_manager_class is async so we might need to update all the tests that involve a contents manager. Or alternatively figure a way to provide a sync contents manager.

I'd recommend to not worry about the remaining tests for now, but rather chat a bit more about whether we want/can provide a sync contents manager (assuming e.g. the base class is sync).

[mwouts]

Re creating a class with synchronous methods I have tried this: e198928:

def build_jupytext_contents_manager_class(base_contents_manager_class):
    """
    Derives an sync TextFileContentsManager class from the given base class.
    The base class can either be sync or async.
    """

    AsyncJupytextContentsManager = build_async_jupytext_contents_manager_class(
        base_contents_manager_class
    )

    class SyncJupytextContentsManager(AsyncJupytextContentsManager):
        pass

    for method_name in dir(AsyncJupytextContentsManager):
        if method_name.startswith("_"):
            continue
        async_method = getattr(AsyncJupytextContentsManager, method_name)
        if inspect.iscoroutinefunction(async_method):
            async_method_name = "_async_" + method_name
            original_signature = inspect.signature(async_method)
            setattr(SyncJupytextContentsManager, async_method_name, async_method)

            def get_sync_method(async_method_name, original_signature):
                def sync_method(self, *args, **kwargs):
                    loop = asyncio.get_event_loop()
                    return loop.run_until_complete(
                        getattr(self, async_method_name)(*args, **kwargs)
                    )

                sync_method.__signature__ = original_signature
                return sync_method

            setattr(
                SyncJupytextContentsManager,
                method_name,
                get_sync_method(async_method_name, original_signature),
            )

    return SyncJupytextContentsManager

but then I run into RuntimeError: This event loop is already running. Do you think that's a direction worth exploring?

[mwouts]

I've been thinking about this and I am tempted to try the following:

• keep your current build_jupytext_contents_manager_class and rename it to build_async_jupytext_contents_manager_class, move it to an async_contents_manager.py file. That function will take as input an async contents manager, and return an async text contents manager
• write a test that copies async_contents_manager.py to contents_manager.py and removes everything relating to async - this way we get a second function build_jupytext_contents_manager_class that takes as input a synchronous contents manager, and returns a synchronous text contents manager. While this approach does duplicate the code, at least we make sure that the duplicated version is up-to-date (the test will check that).

Let me know what you think!

=> I've created that PR: #1329 (did not have the time to run all tests yet)