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

Add local_imports context manager (allow to disable tunnelled imports) #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amigrave
Copy link
Contributor

@amigrave amigrave commented Dec 5, 2017

Example:

def remote_function():
    with chopsticks.tunnel.local_imports():
        import psycopg2
    ...

@lordmauve I admit the name local_imports is not right. Technically, using it make the remote machine use local imports (relative to itself). But when the developer is reasoning about chopsticks code, one might argue that he wants to use the remote imports (relative to it's workstation), hence might find remote_imports more relevant. This make me think we should use a completely different name.

If you're not against this patch, may I ask you some guidance about a good name to use for this context manager, then I'll update my branch. Thanks!

Example:

    def remote_function():
        with chopsticks.tunnel.local_imports():
            import psycopg2
        ...
@amigrave
Copy link
Contributor Author

amigrave commented Dec 5, 2017

@lordmauve Also, I just realize this after making the pull request but in some case, one might want to wrap a whole function with the context manager. In this case I should add an exception to this local_imports thing in order to always use tunneling for module chopsticks.*
I'll add this according to your review,

@amigrave
Copy link
Contributor Author

amigrave commented Dec 5, 2017

... and I could also add some information about this context manager + an example in doc/howto.rst

@lordmauve
Copy link
Owner

This is a good idea. I'm not sure about the context manager though. We could add this as a setting in the chopsticks namespace, eg.

chopsticks.IMPORT_LOCAL = ['psycopg2']

There's already some code for transferring these settings (DEPTH_LIMIT) to remote hosts.

@amigrave
Copy link
Contributor Author

amigrave commented Dec 5, 2017

The settings in the chopsticks namespace is cool indeed!
That said, the context manager allows conditional handling during runtime and with it you can also decide what to do in case of ImportError (I'm doing crazy stuff with chopsticks : ) . Would you agree for both ?

I'll have a look tomorrow about the settings chopsticks.IMPORT_LOCAL, I have to check the import hook more deeply because the challenge is to apply the directive to a list of module and all their dependencies, I don't even know if that will be possible.

See, in my case I want to import odoo locally with it's dependencies, but I don't want to have to insert all it's dependencies in chopsticks.IMPORT_LOCAL.

PS: are you ok with the local_import name ?

@amigrave
Copy link
Contributor Author

amigrave commented Dec 6, 2017

@lordmauve I don't see an easy way to make the whitelist chopsticks.IMPORT_LOCAL behave like the context manager (concerning dependencies). The only way I think of would be to introspect the parent stack frame in order to extract the __name__ global and keep a tree of dependency in order to know what module is a dependency of another module in chopsticks.IMPORT_LOCAL.

Not impossible but pretty ugly and overkill don't you think ?

So if I summarize properly, the options are:

  1. keep the context manager alone
  2. keep the context manager and add the chopsticks.IMPORT_LOCAL directive which will work only with a list of modules and not inferring dependencies like the local_import context manager does (It think this option is pretty decent if described properly in the doc)
  3. do the dependency tree management by introspecting the execution stack frames
  4. you pop out a wonderful idea out of your fez :-)

@lordmauve
Copy link
Owner

Maybe I'm misunderstanding the requirement, then. You want certain modules and everything they depend on to be imported "locally"? This runs the risk of importing different versions of libraries on different remote hosts, which on the whole I think might be best avoided. The advantage of the setting is that you can name specific packages to be imported from the local sys.path without potentially picking up anything else from the local path.

@amigrave
Copy link
Contributor Author

amigrave commented Dec 6, 2017

Maybe I'm misunderstanding the requirement, then. You want certain modules and everything they depend on to be imported "locally"?

That's it ! (in some cases at least)

Most of the cases could be (and would better be) handled by the chopsticks.IMPORT_LOCAL directive.
Eg: a module with compiled code such as lxml, psycopg2, ...
I completely agree that we need chopsticks.IMPORT_LOCAL because you define those import exceptions out of the code itself. Much cleaner.

But in some case you might want to execute python code using a locally installed python stack in a remote machine, container, ...

Let's replace my odoo example with django.
You'd might want to execute some code which import django and then do some processing. In this case you want the import of your stack to be done with the modules of the remote machine. Of course you have to use compatible python versions but you are already doing it for chopsticks to work.

Hence:

with local_imports():
    import django
    import some_other_stack_modules
# ...continue your stuff and use your stack...

I guess one could spawn another Local tunnel inside an SSHTunnel and inject site packages dir before chopsticks:// but that requires to make an outer function in order to spawn the original one in addition to spawning 2 processes for something that could be made with only 1, and it also adds troubleshooting complexity where not needed (and to be honest, my attempt with this approach failed so far but I didn't investigated further).

@amigrave
Copy link
Contributor Author

Hi @lordmauve ,

It seems that you don't like the idea of the context manager, could you confirm ?
If so, I don't care about having a separate chopsticks helper module in my project but I'll still need the few lines of code allowing to do so with the local_imports global or whatever name you prefer.

So if you want me to implement chopsticks.IMPORT_LOCAL as you proposed and remove the contextmanager just tell so I can continue in my project cleaning.

Thanks.

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