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

Better Python code reuse #1271

Closed
sztomi opened this Issue May 8, 2017 · 45 comments

Comments

Projects
None yet
9 participants
@sztomi
Copy link
Contributor

sztomi commented May 8, 2017

Currently, it is possible to reuse Python code by requiring it and tools.pythonpath. This is a bit limited because it's not possible to import the module before the ConanFile is loaded, which makes it impossible to do some things (such as subclassing ConanFile). I tried with a profile as well:

[build_requires]
mypymodule/1.2.3@sztomi/stable

.. but this doesn't work either. Maybe it would make sense for build_requires in profiles to be parsed before the conanfile is loaded. Or a separate, special section just for python deps of the conanfile. Is there a better way? What are your thoughts?

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented May 8, 2017

You are right. We were thoroughly considering this issue when we added support for reusing python code. The thing is that it is a chicken and egg problem, it doesn't seem possible, as you first need to parse the conanfile.py to know the requirements. The [build_requires] in profiles is just a case, but conan will be providing in next release build_requires and build_requirements() in conanfile.py, as they are necessary. So you cannot know the build requirements without loading and parsing the conanfile, and if the conanfile uses code of the build requirements.... Parsing just a part of the conanfile.py to extract that info, apply and later parse the rest, seems like almost impossible to me, it would be too much complex, I don't see any reasonable technical approach to this problem :(

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented May 8, 2017

Would adding a py_requires and special-casing it to work only with the configparser be a bad trade-off (i.e. conanfile.txt or profile)? After all, it would make this possible without disrupting too much else.
Another possibility would be to have an optional config section on top of conanfile.py (don't mind the syntax choices, it's just to convey the idea):

# conan
#
# [py_requires]
# mypymodule/1.2.3@sztomi/stable
#
# /conan
#

from mypymodule import SuperConanFile

class MyConanFile(SuperConanFile):
    pass

This config section could be parsed and taken into effect before loading the conanfile.py.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented May 8, 2017

Still seems that it would require too much hacking on the codebase, but honestly, no idea how to approach implementation of such 2-phase parsing. I will have a look when possible to come up with some analysis of the required effort and complexity. Thanks!

@absassi

This comment has been minimized.

Copy link
Contributor

absassi commented May 10, 2017

Since I'm interested in this feature too, I'd like to propose an alternative, perhaps easier to implement, which is simply a tools.require() function that would get the requirement (if not cached yet) and update sys.path:

from conans import tools

tools.require("mypymodule/1.2.3@sztomi/stable")
from mypymodule import SuperConanFile

class MyConanFile(SuperConanFile):
    pass

Or maybe as a context manager to make it more clear the requirement is just for the imports in conanfile.py:

from conans import tools
with tools.require("mypymodule/1.2.3@sztomi/stable"):
    from mypymodule import SuperConanFile

The tools.require() should, of course, keep track of the dependency graph to detect incompatible version requirements too.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented May 10, 2017

Hi @absassi

Thanks for the suggestion. Yes, the interface would be easier that way.

However, my main concerns are not about the interface or its declaration, but all the internal flows. For example, can those requirements be overridden by downstream declarations, willing to force an update to latest versions?
In the current implementation, many things from the recipe are evaluated before actually resolving requirements, like constructing the initial requires list, evaluating configure() and config_options() method, composing and evaluating current values for setting and options, evaluating requirements()` method, that can be conditioned to setting and options values, etc. Also build_requiresand in next release,build_requirements()`` method.

So we need to parse lots of parts of the class MyConanFile before the actual dependencies/requirements of the package are known. In order to be able to inherit from a class contained in another conan package, you must skip all that related functionality. With build_requires is kind of simple, because they are evaluated very lately in the process, just when building from sources and the full recipe has already been evaluated and the dependency graph built.

So for some solution that could implement this dependency, it would had the following implications:

  • No conditional requirements.
  • No upstream configurability, i.e. the dependencies cannot have options, because they cannot be set consistently from downstream, as the final options values for the consumer haven't been evaluated yet.

From the implementation point of view, I see a lot of complexity. It is not simple to directly hook all the conan functionality inside a call to tools.require(), note that this can require a full graph computation, the retrieval or building from sources of packages... So it should be more like the build_requires, conan should do a two phase parsing of the conanfile.py, extract the py_requires, execute them, and inject them to the final parsing of the conanfile.py. It will require changes to the conan codebase and architecture even bigger than the build_requires feature. So IMHO, it seems to much of an investment for a non extremely critical feature. I would appreciate to know the amount of python code that has to be duplicated due to the lack of this feature (considering that python code can already be reused via composition inside many methods), for further consideration.

Also, as another simple alternative, why not considering a two step workflow:

  • conan install mytools/.... -g env #that will include python packages, using the PYTHONPATH
  • activate.bat # or source activate.sh
  • conan install # project, recipes will find SuperConanFile module and class in the path.

I am not opposed to this feature, and conceptually it is mostly fine. But I am concerned about the implementation complexity, much more code to maintain, to test, to understand for new contributors, and most likely new bugs and regressions that will be introduced. So it is something that we must carefully consider.

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented May 11, 2017

two step workflow

That's what we are doing now. The only drawback is that "mytools" can't import SuperConanFile.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented May 11, 2017

Sorry @sztomi I don't understand, what do you mean that "mytools" can't import it? Do you want also the build tools to depend on the SuperConanFile? That would be the meta-chicken-egg problem :) :) :)

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented May 11, 2017

Yes, I used "mytools" from your example above:

conan install mytools/.... -g env #that will include python packages, using the PYTHONPATH

Yes, this is a chicken-egg problem, my proposals were about circumventing the issue by adding a pass before loading the file.

@absassi

This comment has been minimized.

Copy link
Contributor

absassi commented May 11, 2017

@memsharded, I think you misunderstood the functionality of my proposed tools.require(). It does not (and must not) depend at all of the conanfile currently being parsed, since that's the limitation @sztomi was talking about. The idea is to be able to get parsing dependencies before requiring parsing, so it must not appear inside ConanFile class - and I suggest using pure Python (i.e., a function call from the always-available conans) instead of another file to specify py_requires.

I believe this idea is semantically correct, since these dependencies are not requirements of the package, they're requirements for the conanfile of the package, and thus they should not be controlled by the contents of ConanFile class, should not be visible or controlled anyway from downstream and they should not view or control any of the upstream requirements of the package (it may do so for the requirements of the parsing requirement it is importing, but that seems overkill for now).

Regarding implementation, it is indeed a complex operation, but the operation is already ready to be used: it is just have to download the package using Conan (if not already in cache) and update sys.path. I believe it is safe to assume the package being required is pure-python and therefore not requiring a build step or anything else, but on the other hand, a full-featured conan install running under the hood might be as easy to implement as just fetching packages. When user calls tools.require("mypymodule/1.2.3@sztomi/stable"), it should:

  • call the functions that are equivalent to run conan install mypymodule/1.2.3@sztomi/stable
  • obtain the PYTHONPATH from the package_info of the installed package
  • add it to sys.path or return a context manager like tools.pythonpath does

Options and settings, if that makes any sense for a Python requirement (C extension modules?), may be specified as extra arguments to tools.require. One little extra care must be taken regarding settings, since it does not make sense to use the same that are being injected to the package: suppose I'm cross-compiling some C package and its conanfile.py requires a Python package with a C extension, I want to build that extension to run in my machine, not on the package's target. To solve this, my suggestion is to simplify: for a first version of tools.require do not make options available and forbid installing packages that specify any settings. This is OK for all real use cases - we are only limiting the way one can specify how to build extension modules for reusable conanfiles. Or perhaps disable building at all in this case.

Thanks

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented May 11, 2017

Ok, @absassi, thanks very much for the detailed explanation.
Yes, I like your reasoning and mostly agree about the settings and options. But not sure about it being absolutely invisible from downstream, so I would love to get further feedback from @tru, @sztomi, @lasote, @sixten-hilborn too.
Thanks!

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented May 12, 2017

Yep, @absassi described it very well, and his proposal is better than mine. I'd prefer the version without the context manager though.

@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented May 16, 2017

I understand the issue and appreciate the detailed comments proposing a solution but I think that we should keep thinking. I'm afraid that tomorrow we find a use case that needs to have the full dependencies implementation for the python requires. It doesn't feel clean.

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented May 16, 2017

I'm afraid that tomorrow we find a use case that needs to have the full dependencies implementation

The proposal from @absassi does implement full dependencies, doesn't it?

@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented May 16, 2017

and thus they should not be controlled by the contents of ConanFile class, should not be visible or controlled anyway from downstream and they should not view or control any of the upstream requirements of the package

for a first version of tools.require do not make options available and forbid installing packages that specify any settings.

I'm not telling that those are not good ideas or good proposals, I mean that I don't like to have different flows or different kind of requirements.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Oct 27, 2017

I was thinking about:

  • Automatically adding /.conan/python to PYTHONPATH
  • Users can sync common code with conan config install

Of course the problem with that is that it won't be versioned, in fact it is a totally alternate flow outside of the conan dependencies resolution, but is very simple to implement.

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented Oct 27, 2017

What prevents conan from doing a full dependency resolution for a tools.require-kind solution as suggested by @absassi ?

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Oct 27, 2017

Just implementation complexity, I cannot see a way that is not very complex. It is easy on the UI, but making the interpreter evaluate that at loading time is quite a challenge. Note that all operations are driven by conan, you load the python module, then execute each methods. With the proposed approach you have to invert control, the python interpreter would have to take control and resolve everything while loading the conanfile.py. Seems quite difficult for me, but I could have a second (or third) look

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Feb 28, 2018

Agreed with @absassi that this should have very limited scope:

  • call the functions that are equivalent to run conan install mypymodule/1.2.3@sztomi/stable
  • obtain the PYTHONPATH from the package_info of the installed package
  • add it to sys.path or return a context manager like tools.pythonpath does

I don't like the conan config install approach because it's just copying files around - they will get out of sync and are not tied to a package.

I also don't like this approach

conan install mytools/.... -g env
activate.bat # or source activate.sh

because it depends on the developers to do the right thing, otherwise Conan commands start failing in ways that are difficult to understand (mypymodule not found). Having the functionality built into the profile means it will "just work".

@marausch

This comment has been minimized.

Copy link

marausch commented Apr 6, 2018

Just one observation about the ConanBase example.
The key of this approach is in using the exports attribute which makes the recipe self-contained.
But there is a limitation given by the way exports is working, because the exported file (base_conan.py in the example) needs to be besides conanfile.py in order to be copied besides it in the cache too.
It would be useful if we could put base_conan.py to a common location (the same for all our different conan packages) and get it copied besides the conanfile.py into the recipe's location in the cache.
If this is already possible, what am I missing?
TIA

@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented Apr 6, 2018

Well, if you have your python files in a folder and set the environment variable PYTHONPATH (in the shell you are launching conan, for example) pointing to that folder, conan should be able to locate them.

@marausch

This comment has been minimized.

Copy link

marausch commented Apr 8, 2018

By setting the environment variable PYTHONPATH to point to the common folder where base_conan.py is located, conan is indeed able to find it (base_conan.py is correctly imported by conanfile.py), but

    exports = "base_conan.py"

doesn't yield the wanted result: base_conan.py doesn't get copied to the cache along with conanfile.py.
In fact I expected this to happen, and the question is actually "how should I specify that I want conanfile.py to be exported from the common folder"?
Specifying the path like this:

    exports = <???>/"base_conan.py"

doesn't suit because with 'exports' paths are kept (and in any case the common folder is not a subfolder of the conanfile's location).

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Apr 8, 2018

You can use

exports = "../some_dir/base_conan.py

and it will copy base_conan.py to the directory where your conanfile.py is (it doesn't keep the path if the file is relative to the parent directory).

That works, the downside being you need to put that exports in all of your conanfile.py scripts. It's a good example of something you would want to have factored out into your base class, but in this case it's not really feasible :)

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Apr 11, 2018

We ended up using a PYTHONPATH-based approach because we didn't want every conanfile.py to have to specify an exports. That's something that should be factored out IMO.

We would ultimately like an enhancement to make this approach cleaner. @absassi proposed one earlier in this thread that I like.

@absassi

This comment has been minimized.

Copy link
Contributor

absassi commented Apr 11, 2018

Sorry for taking so long to reply to this thread again, but regarding implementation of my proposal, which seems to be liked by many, @memsharded commented that it would be a challenge because of the inversion of control, but I though it wouldn't be much hard to implement the first step, because in my mind it would be something as simply as calling conans.client.conan_api.Conan.factory()[0].install(). Or even just a subprocess.call().

I don't think it is necessary or even a good idea to try to use currently running Conan instance to install the package, because currently running Conan might be doing something completely different and it would be easy to mess with its current state.

Of course, there are many details to take care to spawning a sub-instance, such as forwarding errors to parent instance, but I guess they shouldn't be that complicated.

Please excuse me if I am oversimplifying things...

@marausch

This comment has been minimized.

Copy link

marausch commented Apr 11, 2018

What bout an approach like this?

File base_conan.py contains:

import os
import inspect
_base_conan_rel_path = os.path.relpath(os.path.abspath(inspect.getsourcefile(lambda:0)))

from conans import ConanFile

class ConanBase(ConanFile):
   exports = _base_conan_rel_path

and file conanfile.py contains:

from conans import ConanFile

sys.path.insert(0, os.path.curdir)
from base_conan import ConanBase

class ConanFileToolsTest(ConanBase):
    name = "test"
    version = "1.9"

This is taken from the mentioned example modified according to the solution using relative paths.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 11, 2018

Please excuse me if I am oversimplifying things...

Yes a bit :) :). Well, it depends on the scope... If you want to have the same dependency resolution, transitivity of options and conflicting options, affecting the package binary ID, etc., then yes, it is challenge, and a lot of work.

But I have been considering a more pragmatic alternative, in the line of what @marausch suggest. Might try something very simple: no options, no packages, just importing python code from a depended conanfile.py. I have done a small proof of concept and seem feasible. I will try to come up with something to share in the following weeks. Thanks all for the feedback!

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 19, 2018

I have been thinking about this a bit, and checked a couple of things. As said above, the full case is extremely challenging. But I think that a pragmatic approach might be useful for 99% of cases. My suggestion would be:

from conans import ConanFile, conan_python_require

base = conan_python_require("MyConanfileBase/1.1@user/testing")

class PkgTest(base.MyConanfileBase):
    pass
  • Requirements cannot have options or even settings, as they will be just pure python code.
  • The dependency "MyConanfileBase/1.1@user/testing" is not even a package. It is imported directly from the recipe itself. So it is not necessary to create the package, just export + upload the recipe.
  • Yes, the same recipe exported for "MyConanfileBase/1.1@user/testing" is the one that is imported with conan_python_require.
  • It can contain other functions, not necessarily to be reused as a BaseClass
  • It won't affect the packageIDs at all.
  • They are private to the recipes, do not propagate downstream or upstream.
  • It can be very fast, as not creating packages, not messing with PYTHONPATH, caching already imported python code.

Some open issues:

  • It cannot parameterize using self.user, self.channel as self is not defined yet. Check if they could be injected to the module instead. Or assume they don't neet to change user/channels as normal library packages, as they do not form part of the output, but from the tooling.
  • I have the doubt how to manage overriding from downstream. As it will be hardcoded in the recipes, it should be possible to override them just by using an upgraded conan_python_require("MyConanfileBase/2.0@team/stable") from a consumer recipe? It can be done, if assuming that the only relevant thing is the MyConanfileBase package name.

This should be relatively easy to implement, but I would need a strong evidence from the majority of users that this would be everything that is really needed. Feedback please @solvingj @grafikrobot @sztomi, @sourcedelica @absassi @tru. Many thanks!

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Apr 19, 2018

This works great for my use case. Thanks @memsharded!

I'm not sure what you mean by the second open issue. Could you describe it some more?

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 19, 2018

Yes. You have in your recipe:

from ..

base = conan_python_require("MyConanfileBase/2.0@team/stable")

class Pkg(base.MyConanfileBase):
   ...

In every of your 100 recipes. Now you release ``"MyConanfileBase/2.1@team/stable"```. How do you make your packages to use it? Editing all of them? Or you should be able to define from downstream just once?

@grafikrobot

This comment has been minimized.

Copy link
Contributor

grafikrobot commented Apr 19, 2018

@memsharded Looks like a splendid idea. But some questions in trying to understand the scope of this it:

  1. Would the usual version range specifications work on this?
  2. Would multiple conan_python_require statements be allowed?
  3. Would the code in the conan_python_require "package" contain any Python definitions, not just a ConanFile?

In an ideal world I would like the answer to all of those be yes ;-)

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Apr 19, 2018

Or you should be able to define from downstream just once?

Yes, that would be wonderful. Above and beyond what my expectations would be, but great nonetheless :)

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 19, 2018

@grafikrobot The answer to 2, 3, is yes. For 1, it is a good point, I haven't thought about it, I should check, but maybe it is possible too.

@absassi

This comment has been minimized.

Copy link
Contributor

absassi commented Apr 20, 2018

@memsharded that's excellent for me.

Regarding your example of upgrading your 100 files from 2.0 to 2.1, I'd say it is the safe approach is to avoid downstream overriding, because these requirements are directly related on how the recipe is interpreted and have it overridden downstream would open doors for strange and hard to find bugs in user recipes without actually solving the actual problem that is that these 100 files are really outdated now. Version ranges resolve most of this issue pretty well, so I'd go for that instead of downstream overriding first.

One question that I only though about now: will this work well with multiple inheritance? Because I see now that once this is available, many will create lots of mixin classes and it would be really nice it this works too. I guess that if you guarantee that the conan_python_require always return the same module instance for the same requirement, then it works.

@marausch

This comment has been minimized.

Copy link

marausch commented Apr 20, 2018

Let me say that the proposed solution is excellent for me too (even if I'm not in the list of those required to give feedback).

"Guarantee that conan_python_require always returns same module instance for same requirement":
this is what fulfills the requirement to makes the recipe self-contained (or behaving as if it were).
Point 1 about version ranges is a useful extension.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 20, 2018

Ups, sorry @marausch I intended to add you to the list for feedback, I somewhat skipped while writing.

Thanks for the feedback! It seems there is good support for this. I will try to move it a bit forward investigating about the version-ranges, and might get back after 1.3 with a proof of concept.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 20, 2018

@absassi I understand your point about overriding vs version-ranges, but I am not sure if that is the preferred approach by the majority of users. Maybe make some opt-in/opt-out to the behavior?

I think multiple inheritance will be possible, I'll check it too.

@sztomi

This comment has been minimized.

Copy link
Contributor

sztomi commented Apr 20, 2018

We also discussed the proposal with @tru and it seems that it would fit our use-case very nicely. As for the updating-the-version-downstream issue, we decided that we actually don't need that, in fact, we prefer no magic at all to accomplish it. We already have a script that updates our package revisions so we will probably teach that to update these python package references as well. Then we commit it as if it was updated manually for all of our packages. The advantage of this solution (compared to any kind of dynamic updating, such as an environment variable) is that for any given SHA we get a package and an exact python package reference, i.e. the build remains reproducible.

@grafikrobot

This comment has been minimized.

Copy link
Contributor

grafikrobot commented Apr 20, 2018

Additional concern for when this is available.. Since this looks like a non-backward compatible change for recipes that use this feature; Will there be a way to add a conan version requirement to the recipes?

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 20, 2018

Will there be a way to add a conan version requirement to the recipes?

Good point. Just a reminder that there are already features in 1.2 and 1.3 that are backwards incompatible. Stability and compatibility can be only provided forward: http://docs.conan.io/en/latest/introduction.html#stable Recipes that are using new features introduced in 1.2 or 1.3 won't run with 1.0. The conan version (I read it as a "this recipe was written in conan 1.2, and that is the mininum version of conan needed to run") would only provide nicer error messages, but besides that, can't do much. So I'd say this is an orthogonal feature, independent of the python_requires anyway.

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Apr 20, 2018

@sztomi Thanks for the feedback. Then, we'll definitely add some opt-in/opt-out behavior, to ensure you can achieve the behaivior you describe.

@DoDoENT

This comment has been minimized.

Copy link

DoDoENT commented Aug 2, 2018

As conan_python_require is not yet part of conan 1.6.1, I have added my base_conan.py to public Github repository and download it on every conan install with following code:

from conans import tools

tools.download('https://raw.githubusercontent.com/myorg/my-public-repo/master/base_conan.py', 'base_conan.py', overwrite=True)

from base_conan import BaseConanFile

class MyConanFile(BaseConanFile):
    pass

It's dirty but works like a charm. It also enforces usage of the latest version of base ConanFile, since I download the file from the latest master branch.

You also need to add base_conan.py to .gitignore to your repositories containing your recipes, as conan install ... will download base_conan.py in the folder where conan install is invoked.

I hope this helps someone...

@memsharded

This comment has been minimized.

Copy link
Contributor

memsharded commented Aug 27, 2018

This has been implemented in #3337, and will be then available (as experimental feature) in conan 1.7. Please stay tuned for the release, and looking forward your testing and feedback.

@memsharded memsharded closed this Aug 27, 2018

@sourcedelica

This comment has been minimized.

Copy link

sourcedelica commented Aug 27, 2018

Hallelujah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment