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

submodule working in both notebooks and python files. #6

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

Conversation

sannant
Copy link
Contributor

@sannant sannant commented Jun 16, 2021

if you import the "nested" submodule, the dtool_api works both in python scripts and jupyter notebooks.

At least in simple cases.

TODO:

  • wrap all functions, not only query

@sannant sannant changed the title DRAFT: submodule working in both notebooks and python files. submodule working in both notebooks and python files. Dec 9, 2021
@sannant
Copy link
Contributor Author

sannant commented Dec 9, 2021

I have been using this successfully for a little while now so I think it would be useful to merge this feature now. Could be usefull to others.

@sannant sannant requested a review from jotelha December 9, 2021 08:59
@jotelha
Copy link
Member

jotelha commented Dec 9, 2021

Sure.

That does not necessarily aim at Jupyter notebooks only, but at any environment with an event loop already running, right?

You said thsi works for simple cases, are there cases where it won't work?

Can we auto-wrap this? Lars already did that for the wrappers in dtool.synchronous, 8e97a8c#diff-11ed3aca09350c3657c911d3595ac87fa692369b6f220ba411488997275b3904

The whole purpose of this here is not to have to differentiate between synchronous and asynchronous when importing, right?

@sannant
Copy link
Contributor Author

sannant commented Dec 9, 2021

You said thsi works for simple cases, are there cases where it won't work?
I didn't observe any, but I didn't try too many neither.

Yes I guess we can autowrap, I was just too lazy until now. I think it shouldn't be too difficult to apply Lars' automatic wrapping.

@sannant
Copy link
Contributor Author

sannant commented Dec 9, 2021

The main reason is that I sometimes open python files either with a normal python interpreter or with jupyter with jupytext.

With this nested API I can seemlessly jump between these contexts

@jotelha
Copy link
Member

jotelha commented Oct 4, 2022

Let's auto wrap with the likes of

import inspect

from .core.LookupClient import ConfigurationBasedLookupClient


class _WrapClient:
    def __init__(self, name, func):
        self._name = name
        self._func = func
        self.__doc__ = self._func.__doc__

    async def __call__(self, *args, **kwargs):
        async with ConfigurationBasedLookupClient() as lookup_client:
            return await self._func(lookup_client, *args, **kwargs)


# Import all methods from ConfigurationBasedLookupClient into the global namespace
for name, func in inspect.getmembers(ConfigurationBasedLookupClient, predicate=inspect.isfunction):
    # Import everything that does not start with an underscore
    if not name.startswith('_'):
        globals()[name] = _WrapClient(name, func)

The nested module introduces another dependency, https://pypi.org/project/nest-asyncio/

I'd suggest to install the nested module and that dependency as "extra" (https://peps.python.org/pep-0508/#extras, https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies) only if explicitly specified with

pip install dtool-lookup-api[nested]

For this, we need to add something like this

extras_require={
        "nested": ["nest_asyncio"],
    },

to setup.py. Alright?

@sannant
Copy link
Contributor Author

sannant commented Oct 4, 2022

Ok, thanks for the snippet. But I don't know when I will look into this again, since it does the job for me and I do not understand the tests. Feel free to take over.

@sannant sannant self-assigned this Sep 25, 2023
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