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

Impossible to use plugins following cli documentation #56

Closed
jredrejo opened this issue May 7, 2019 · 4 comments · Fixed by #63
Closed

Impossible to use plugins following cli documentation #56

jredrejo opened this issue May 7, 2019 · 4 comments · Fixed by #63
Assignees
Labels
bug Something isn't working

Comments

@jredrejo
Copy link
Contributor

jredrejo commented May 7, 2019

Describe the bug
According to https://github.com/zalando-incubator/Transformer/blob/master/docs/Using-plugins.rst, together with https://transformer.readthedocs.io/en/latest/Writing-plugins.html#name-resolution , when using the cli,
transformer -p mod.sub har/ >loc.py
should work if mod/sub.py and mod/__init__.py files exist.
However, it triggers this error
ERROR Failed loading plugins: No module named 'mod'

Transformer version
1.2.3

To Reproduce
Steps and input files to reproduce the behavior:

  1. Anywhere in your system, creates a dummy.py plugin file
  2. Run Transformer with the -p python_path argument
  3. See error

Expected behavior
Transformer should be able to find the plugin.
This is an easy fix for the problem:

--- a/transformer/plugins/resolve.py
+++ b/transformer/plugins/resolve.py
@@ -1,6 +1,8 @@
 import importlib
 import inspect
 import logging
+import os
+import sys
 from types import ModuleType
 from typing import Iterator
 
@@ -27,6 +29,7 @@ def resolve(name: str) -> Iterator[Plugin]:
     :raise InvalidContractError: from load_load_plugins_from_module.
     :raise NoPluginError: from load_load_plugins_from_module.
     """
+    sys.path.append(os.getcwd())
     module = importlib.import_module(name)
 
     yield from load_plugins_from_module(module)

but I am not sure if the intention of the project is providing this feature or change the docs to force to use plugins only after having been installed using pip, thus available in the sys.path.
In case the above solution is acceptable, I can fill a new PR, or feel free to use it.

Desktop (please complete the following information):

  • OS: Ubuntu
  • Version: 18.04.02
@jredrejo jredrejo added the bug Something isn't working label May 7, 2019
@jredrejo jredrejo changed the title Incorrect cli usage documentation Impossible to use plugins following cli documentationi May 7, 2019
@jredrejo jredrejo changed the title Impossible to use plugins following cli documentationi Impossible to use plugins following cli documentation May 7, 2019
@thilp
Copy link
Member

thilp commented May 8, 2019

Hi @jredrejo, thanks for reporting this problem!

We discussed a bit about it with @bmaher and it looks like we may want to generalize it: users probably don't want to install their plugins (or set PYTHONPATH) and they don't want to have to learn about Python modules. This second part comes into play when your plugin is not in your current directory, but in a completely unrelated directory (for which sys.path.append(os.getcwd()) is not enough).

My understanding is that all these should be possible (with an imaginary -P option similar to -p):

$ transformer -p mod.sub har/ >loc.py  # "mod.sub" installed in PYTHONPATH
$ transformer -P ../../plugins/mod/sub.py har/ >loc.py  # use an arbitrary Python file

In the mean time, we should definitely update the documentation. Writing plugins § Name resolution already explains the catch:

Let’s also assume that your Python import path is configured so that you can execute from mod.sub import my_plugin successfully.

But this should be made clearer and repeated, moved, or at least linked in Using plugins § Command-line usage as well.

@jredrejo
Copy link
Contributor Author

jredrejo commented May 8, 2019

Installing the plugin in the python path works, but I won't use it until I have finished developing the plugin and do its python setup.py installation.
By now, I want to execute an arbritary python file as mentioned.
If the new -P option is added, it's great.

However,

In the mean time, we should definitely update the documentation. Writing plugins § Name resolution already explains the catch:

Let’s also assume that your Python import path is configured so that you can execute from mod.sub import my_plugin successfully.

is not correct. If the plugin is in my current directory, it does not work either. Being in the same directory where I execute transformer I can execute without any problem
from mod.sub import my_plugin or from sub import my_plugin if sub.py is in my directory because . is in my path, but it's not in transformerpath, so the metioned catch does not work.

Trying:

$ transformer -p mod.sub har/ >kk.py

works applying the code patch written in this issue description.

@thilp
Copy link
Member

thilp commented May 12, 2019

Maybe related to #39.

@tortila tortila self-assigned this May 13, 2019
@tortila
Copy link
Contributor

tortila commented Jun 7, 2019

@thilp @jredrejo I thought we would be able to solve this with #39, but after my initial work on that it appears that loading of the plugins (i.e. the strategy) still relies on Transformer implementation. So we will have to handle the discovery of the modules anyway. That being said, I imagine there are different things we could do within this issue:

  • fix as suggested by @jredrejo (extending syspath)
  • providing clearer documentation

I suggest taking care of this separately and before #39 is fixed, mainly because #39 will introduce breaking changes to the current plugin system. I imagine that along with #39 we will warn about deprecating the old plugin system, and remove it altogether not long after that. It still make sense to support old plugin system, but only in the old versions of Transformer, so with limited scope.

What do you think? If you agree, I encourage @jredrejo to create a PR with his suggested solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants