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 catkin_install_python function (fix #573) #574

Merged
merged 1 commit into from
Jan 6, 2014

Conversation

dirk-thomas
Copy link
Member

@tfoote @wjwwood Please review.

@wjwwood
Copy link
Member

wjwwood commented Jan 6, 2014

+1 from code review

@tfoote
Copy link
Member

tfoote commented Jan 6, 2014

+1

dirk-thomas added a commit that referenced this pull request Jan 6, 2014
add catkin_install_python function (fix #573)
@dirk-thomas dirk-thomas merged commit fe2910e into groovy-devel Jan 6, 2014
@dirk-thomas dirk-thomas deleted the python_install_shebang branch January 6, 2014 23:28
@jack-oquin
Copy link
Member

Shouldn't ROS scripts be able to support this recommendation of PEP-394?

  • in preparation for an eventual change in the default version of Python, Python 2 only scripts should either be updated to be source compatible with Python 3 or else to use python2 in the shebang line.

I don't completely understand the CMake code for bbf8192, but it appears to rewrite the shebang line unconditionally. If so, how do users describe which Python version(s) they support?

@dirk-thomas
Copy link
Member Author

The custom install function rewrites a shebang line if it matches the following line (bbf8192#diff-3cfcccb0a4b1968a8dae7de378653f99R31):

#!/custom/path/to/env python

The developer has the choice to either use that new function to rewrite the line to a specific python version or stick to the CMake function install to install the script without shebang rewriting. Anyway if the shebang line already specified a specific Python version (which means it does not match the above pattern) the file is not modified by the new function (which the developer should not use for that kind of file anyway).

So I can't see how this approach does not follow PEP 394. Can you please clarify that, eventually with an example?

@jack-oquin
Copy link
Member

What are we telling ROS users to do?

I thought we wanted them to write bilingual scripts that work with both Python 2 and Python 3. PEP-394 recommends that bilingual scripts use the shebang name "python", so they run with the default system interpreter.

I can't figure out what the intent of this change is. Is there documentation that explains when people should or should not use this new command?

@dirk-thomas
Copy link
Member Author

The recommendation will be that every Python script with a shebang line (which is not installed via the setup.py file) should be installed using the new function (instead of the plain CMake install function). And of course that every Python code should work with both Python versions.

As long as you are not building from source and pass in a specific Python version (via -DPYTHON_VERSION=...) the default Python interpreter will be python (as found by find_package(PythonInterp)) so no magic is happening with the shebang line on installation.

If you want to use e.g. Python 3.3 on Trusty (which will be the python3 executable) then you have to build ROS from source and specify that specific version at configure time. The new function will then make sure that the installed scripts will have the correct shebang line. The same applies for platforms where python points to a version 3 interpreter and the user want to run ROS with Python 2 (since some stuff is not yet working well with version 3). Without the new functionality there would be simply no way to run a non-standard Python version.

Since the modification only affects the install space and there is currently no conceptional way to overlay scripts in source space from the devel space using a different Python version will require the user to install. The only way to avoid this restriction is for the user to setup a virtual env with a different default Python interpreter and building / running everything from within that virtual environment.

@tfoote
Copy link
Member

tfoote commented Jan 7, 2014

@jack-oquin The recommendation is to write bilingual scripts. However when they are deployed they no longer are bilingual. Some of them link in c libraries etc which are specific to the version of python. And they install to different locations pythonX.Y. setup.py already does this on installation. This is making catkin/cmake do the same for package local scripts.

This helps write bilingual scripts by making it easier to parameterize which version of python you want your targets to run. Otherwise if you wanted to test on python 3 you would have to violate PEP 394 to make things work. (Note this only fixes the installed case, for devel space you will need to override the python executable in your environment. But we plan to recommend using a virtualenv anyway. And you can't have both be the default for bilingual code anyway. But the installed case is where you may not be able to override your python executable without major reprecussions.)

@jack-oquin
Copy link
Member

Thanks for the explanation.

I eagerly await a tutorial on how to test a ROS Python package with Python 3. So far, nothing I have tried has yielded any useful result in a reasonable about of time.

@dirk-thomas
Copy link
Member Author

@jack-oquin If you want to try Python 3 before a tutorial is available you simply need to switch out your Python interpreter python. You could do this in three different ways:

  • remove /usr/bin/python and instead create a symlink pointing to /usr/bin/python3. This is see easiest but most invasive way to use Python 3. You will likely only want to do this in a clean VM (with the current Trusty images you will have to revert this every time you use system tools like apt-get).
  • instead of changing the Python interpreter globally you can change it just for your current shell. Therefore create a folder /tmp/python3/bin and create symlink to the Python 3 interpreter and name it python. Then prepend this folder to your path.
  • the cleanest way is to use a virtual environment. Then you can also install Python packages from PIP in there and don't have to worry about your actual system. Also in the virtualenv you should map python to a Python 3 interpreter (since existing packages don't allow for the shebang rewriting yet). For the first two bullets you would still need to install the python3-* packages to your system (which will partially conflict with your python-* packages).

@jack-oquin
Copy link
Member

Why do the python3-* packages conflict with the python-* ones?

Specifically, which packages have that problem?

@dirk-thomas
Copy link
Member Author

Any package which installs global binaries, e.g. rospkg which installs rosversion.

@jack-oquin
Copy link
Member

A bit of thought suggests reasonable solutions to problems like that, especially given the fact that the scripts themselves are bilingual.

I note, for example, that on Precise python-scipy installs both /usr/bin/f2py and /usr/bin/f2py2.7, while python3-scipy installs /usr/bin/f2py3.2. All three scripts are identical, except for the shebang line.

Consequently, there is no problem with installing both versions on Precise or with using scipy with either interpreter.

Surely, similar solutions could work for us?

@dirk-thomas
Copy link
Member Author

There are certainly ways to address this problem. Likely not by using a version specific suffix since the script must be available under a certain name. But e.g. the script could be moved to a version-independent package which the version specific packages then depend on.

Anyways I only described the current state. Since nobody has done any effort reworking existing packages like this currently they can not be installed side-by-side.

@jack-oquin
Copy link
Member

A simpler solution, since the scripts are version-independent already, would be for the version-independent package to install its modules for both Python versions.

@dirk-thomas
Copy link
Member Author

Since that would be yet another time ROS does something in a non-standard way we do not want to go that way.

@wjwwood
Copy link
Member

wjwwood commented Jan 7, 2014

@jack-oquin I believe that would require all python packages to depend on Python2 and Python 3. I'm not sure that is desirable, but perhaps there is a way for debian to allow this. However, it seems to be the common case in debian to release separate packages, one for each Python2 and Python3, for all python packages even if they are bilingual.

@wjwwood
Copy link
Member

wjwwood commented Jan 7, 2014

If we want to follow the method used by everyone else, then we will install with suffixes on at least the Python3 specific scripts, but as @dirk-thomas pointed out that is very problematic for us since people commonly reference these scripts by name directly.

One option would be to have Python2 packages install script and script-2.7 and Python3 packages to also install script as well as script-3.3 (assuming there is a way in debian packaging to allow for this) and then have the script file check for a global ROS Python version (global config, environment variable, or something else) and then execute the appropriate version specific script-2.7 or script-3.3 at run time. This is a complicated solution which has some drawbacks, not the least of which is that it would require a lot of work to make sure everything used this strategy in packaging and in their scripts.

It remains an open problem for us and something that think is important to solve, but is not a blocking issue since we will be depending on Python2 (/usr/bin/python) for now.

@jack-oquin
Copy link
Member

Debian and Ubuntu chose to make a clear break between Python 2 and Python 3 packages.

I've been trying to figure out why, since they already had a sophisticated multi-version solution via /usr/share/pyshare, which allowed a single Python 2 package to support multiple versions [2.5, 2.6, 2.7]. I am fairly sure their reason not to continue doing it the same way was because they cannot assume backwards compatibility for all scripts between the major Python versions.

But, we are assuming bilingual scripts. We should be able gain some advantage from that.

Instead, the situation for ROS is much worse, and it still remains impractical to develop bilingual scripts that depend on ROS, even trivially.

@wjwwood
Copy link
Member

wjwwood commented Jan 7, 2014

There is no problem to have one source tree which provides Python packages and modules which are bilingual. These packages can be built from source and work on Python3, that is a huge plus and we highly recommend people do this.

For packaging that may result in multiple debian packages, but that's OK and would be a mostly automated process with bloom and the build farm. The problem comes in with scripts (and other non-python incident files like package.xml and launch files). The problem for scripts, as far as I understand, is not at all addressed by /usr/share/pyshare.

Like I said packaging for Python3 and Python2 simultaneously remains a complex problem, which has no clear solution at the moment. The best thing we can do is observe what the rest of debian is doing and do our best to follow suit. It does not prevent us from moving forward with Python2 as the default packaging for ROS and it does not diminish the ability for people to test their packages with Python3 from source nor does it take away from the value of that testing.

@jack-oquin
Copy link
Member

There is no problem to have one source tree which provides Python packages and modules which are bilingual. These packages can be built from source and work on Python3, that is a huge plus and we highly recommend people do this.

I've been asking for a year now how to do this, with no practical answer.

I am hard at work writing substantial amounts of new Python code for the ROCON project. We develop on Hydro with Precise, most of it will eventually be released on Indigo.

I try to make my code bilingual. But, with dependencies on ROS messages and some basic ROS services there is no practical way to test that, which almost certainly means that it is not bilingual. By "practical", I mean something I can set up in a couple of hours that allows me to quickly and easily run my unit tests in a Python 3 environment after almost every commit. "Practical" does not mean taking a day or two away from my project to figure out how to install Trusty (still not released) in a virtual machine, learn virtualenv, and then figure out how to build ROS from source that way.

Meanwhile, the best I can do is run some low-level test cases with python -3, which complains bitterly about simple incompatibilities in the core ROS scripts that we knew about a year ago (mostly problems with __hash__ and __eq__).

For me to get my work done and not waste time, "practical" probably means ignoring Python 3 until OSRF gets serious enough about it to provide a decent bilingual development environment. I still see no signs of movement in that direction, or even recognition that it is a problem.

@tfoote
Copy link
Member

tfoote commented Jan 11, 2014

@jack-oquin Please remember our posting guidelines of assuming good faith and don't use ad hominem arguments for individuals or groups. http://wiki.ros.org/Support Your comment has hurt quite a few feelings. Many man months at OSRF have been spent working on making ROS compatible with Python 3. We're tracking overall progress here: ros/rosdistro#2427 and we've made a lot of progress.

Converting to python3 is a hard problem with many nuances and we're working to hit a moving target as Ubuntu has not finalized how they are deploying Python and there are many upstream packages that have not even been released for python3 that OSRF has ported and are working to get submitted back upstream. We have not been focused on making an easy way to test code within python3 because the core requirements are not available yet to us or anyone else. Things are changing all the time. Brett Cannon who wrote the python documentation for porting Python2 to Python3 just 2 days ago rewrote the whole document. https://plus.google.com/+BrettCannon/posts/9M2wy1zZ4DM

It's very unlikely that you will be able to test things effectively in Precise, it's was released in early 2012 and python 3 porting recommendations are changing even in 2014. There are 158 python3-* packages vs 1780 python-* packages on my precise machine. We do all our testing with Saucy or Trusty because they have most of the system dependencies we need unlike Precise. Without backports of many of those packages upstream we all will need to pay the price of setting up VMs etc. It's a painful transition and unfortunately we can't move any faster than all our dependencies and we have to follow their precedences on packaging etc.

Here's a good analysis of why things are having trouble converting to python 3 overall: http://blog.startifact.com/posts/python-2-gravity.html

@jack-oquin
Copy link
Member

I am very sorry that my statements hurt your feelings. I expressed myself badly to have caused that undesired and unexpected result.

I assume it was this statement you found most offensive:

For me to get my work done and not waste time, "practical" probably means ignoring Python 3 until OSRF gets serious enough about it to provide a decent bilingual development environment. I still see no signs of movement in that direction, or even recognition that it is a problem.

Please grant me the "assumption of good faith" for a moment. I did not say "no one has been working on Python 3 support in the ROS core". I said the goals of that work do not appear to include providing a "decent bilingual development environment" for other ROS developers to use.

Earlier, William had said:

There is no problem to have one source tree which provides Python packages and modules which are bilingual. These packages can be built from source and work on Python3, that is a huge plus and we highly recommend people do this.

I am reporting the fact that for me this statement is not true. You guys are certainly smarter about this stuff than I am, but we need to be able to explain it to ordinary mortals who have other jobs in addition to playing with Python 3. I have at least made a serious effort to follow this issue. There are probably many others who don't understand it any better than I do.

That is what I mean by "not recognizing that it is a problem". I am not assigning bad faith, but I have come to doubt that my needs and those of others like me are clearly understood by the three of you. You plan to release ROS packages that exclusively support a single Python version on each target system. Maybe that solves the ROS core part of the problem, but it seems to leave the rest of us in limbo.

I worry that the currently-defined goals and requirements for Python 3 migration are going to create a disaster next year. I hope I am wrong. Maybe this is a 2016 problem and I am thinking about it much too soon.

Tully seems to say I should ignore Python 3 in my own code until 2015, that there is currently no way to develop "polyglot" ROS scripts because I have no alternative other than using Hydro and Precise. That may be true. If so, let's not tell ROS developers they should develop polyglot scripts and that everything necessary is in place for doing that.

So, please interpret my remarks as: "Jack does not get it. He does not understand the plan for migrating the entire ROS code base to Python 3."

I hope this is all just a communications problem, but I fear that it may be a more fundamental disagreement about what we actually need to accomplish.

@wjwwood
Copy link
Member

wjwwood commented Jan 13, 2014

There is no problem to have one source tree which provides Python packages and modules which are bilingual. These packages can be built from source and work on Python3, that is a huge plus and we highly recommend people do this.

Let me be more clear about this, I didn't mean to imply that doing the above is easy or there are no problems with asking everyone to do it.

There is no problem to have one source tree which provides Python packages and modules which are bilingual.

What I meant here is that, given you know how to write and test your code with Python2 and Python3, you can have a source tree which supports both versions. Now that is to say that distributing them as both, either as one bilingual package or multiple version specific packages, is not clearly understood yet and we are still working out how to do it properly, but there are no technical hurtles to having a Python2/Python3 package in source.

These packages can be built from source and work on Python3, that is a huge plus and we highly recommend people do this.

Here I was most unclear, I meant that there are no technical barriers to building everything from source to use Python3, even if we don't know how to package them yet. This allows systems which use Python3 as the default to build things from source at least. This is a huge benefit and I would recommend anyone with the expertise and time to do this if possible.

I recognize that there is a problem in asking all ROS Python users to do this before there are tutorials and clear methods for testing their code against both versions of Python. What I was getting at above is that we should write these tutorials and resources once we know what they will contain (as @tfoote said there is not one clear good way to do this yet, we have come up with some solutions which involve virtualenvs and/or VM's, but it remains complicated even for us), but we should not let this block us from making progress on other issues related to spinning up Indigo or getting the core components of ROS to be Python3/2 bilingual.

I am sorry that you feel you cannot easily do Python2/3 testing yet, but we are working on it and we have told you how we do things. If that is not good enough yet, I would recommend you just come back to this issue once we have more details figured out and know that we are working on it.

For reference, most of the new Python work I do right now (capabilities which is used by ROCON), I just write against Python2.7 and where I can remember I make it Python3 friendly so that the code probably still needs some attention before it is completely Python2/3 bilingual, but it is very close. Once all of the core ROS dependencies are Python3 ready then I will turn on Python3 unit tests for my Continuous Integration and polish up the Python3 support. Once I can do that and figure out all the odds and ends, we can write something more concise up about how to do the testing, but as @tfoote mentioned, using Precise and debians is likely to never be a good option. Any instructions we write for testing Python3 will likely end up requiring Trusty or Saucy, a VM with Trusty or Saucy, or use a virtualenv with pip-3.3 and doing a source build of everything.

@jack-oquin
Copy link
Member

I am sorry that you feel you cannot easily do Python2/3 testing yet, but we are working on it and we have told you how we do things. If that is not good enough yet, I would recommend you just come back to this issue once we have more details figured out and know that we are working on it.

I had already reached that conclusion: it's too soon for me to start seriously writing Polyglot code. Like you, I'll do my best, but my Python expertise is only "intermediate", not "advanced", so I will probably make more mistakes than you will.

While we should let people know that Polyglot Python is the recommended language to use with ROS for the next several years, we need to be honest about the testing problems.

It would really help a lot if we could get "python2.7 -3" working cleanly on Hydro. I thought Hydro was claimed to work (from source) on Python 3 systems. Yet, when I try simple low-level unit tests with few dependencies other than ROS messages, I still get warnings like these:

$ python2.7 -3 tests/test_resource_pool.py
/opt/ros/hydro/lib/python2.7/dist-packages/genpy/rostime.py:177: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Time(TVal):
/opt/ros/hydro/lib/python2.7/dist-packages/genpy/rostime.py:281: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Duration(TVal):
/opt/ros/hydro/lib/python2.7/dist-packages/genmsg/msgs.py:158: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Constant(object):
/opt/ros/hydro/lib/python2.7/dist-packages/genmsg/msgs.py:196: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Field(object):
/opt/ros/hydro/lib/python2.7/dist-packages/genmsg/msgs.py:228: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class MsgSpec(object):
/opt/ros/hydro/lib/python2.7/dist-packages/genmsg/srvs.py:43: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class SrvSpec(object):
/opt/ros/hydro/lib/python2.7/dist-packages/genpy/message.py:249: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Message(object):
/opt/ros/hydro/lib/python2.7/dist-packages/roslib/manifestlib.py:242: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Platform(object):
/opt/ros/hydro/lib/python2.7/dist-packages/roslib/manifestlib.py:287: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class Depend(object):
/opt/ros/hydro/lib/python2.7/dist-packages/roslib/manifestlib.py:317: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
  class StackDepend(object):

To my mind, these are bugs which should be fixed, even in released code. I remember opening issues with several components for errors like these a year ago.

If you agree these problems should be fixed in Hydro, I will try to figure out which components are failing and open specific issues for each of them. If I can figure out a fix, I'll even provide a pull request.

@wjwwood
Copy link
Member

wjwwood commented Jan 14, 2014

Some of these things may have been fixed and released very recently, though I'm not entirely sure about this one, maybe @dirk-thomas knows better.

@wjwwood
Copy link
Member

wjwwood commented Jan 14, 2014

This pull request may have the fixes you need to do the -3 option, but I don't know.

ros/ros_comm#305

It is pending more testing and review.

@dirk-thomas
Copy link
Member Author

Hydro does definitely not support Python 3. And very likely that is not going to change. Simply because a very important demand of the community is stability for an already released ROS distribution.

As mentioned before the progress is tracked in this ticket: ros/rosdistro#2427
Some pull request have already been merged (especially when they were small / trivial). Some others are still pending and not yet complete). And when you take a look to the size of the changes e.g. for the ros_comm repo then this is nothing which I would like to merge into Hydro since I can simply not guarantee that this would be regression free.

@jack-oquin Regarding the tickets you mentioned: I can not find any open issue like that in any of these repositories. Please point me to them if I have overlooked them. If they are still a problem and not yet tracked in an open ticket please fill a new ticket and I will look into it.

@jack-oquin
Copy link
Member

@dirk-thomas: I am glad you consider release stability very important. I agree completely.

Certainly no large or risky changes should be made in Hydro. Indigo is the appropriate target for them.

However, fixes for the first two errors I posted (in rostime.py) look trivial and low-risk to me. Basically, we need to define Time() and Duration() __hash__ methods that invoke the TVal() super-class method, like this:

__hash__ = TVal.__hash__

I think we should fix that. If you agree, I'll open an issue and provide a pull request.

@jack-oquin
Copy link
Member

Here's a good analysis of why things are having trouble converting to python 3 overall: http://blog.startifact.com/posts/python-2-gravity.html

@tfoote: That is really an excellent statement of the problem. I would encourage you all to go back and re-read that post from the point of view of ROS package developers. Substitute "ROS core" for "Python core" and you get a better feel for what ROS users and developers are going to be facing.

From our perspective, most of the "Circular Dependency" and "Python 2 Gravity" problems will still remain after OSRF finishes its core ROS migration. Many businesses and research labs have large collections of robots running hundreds of ROS packages. Many are written in Python. Those packages are not all going to have been converted when OSRF releases a distribution on a platform that only works with Python 3, so a very intense "gravitational field" is going to prevent many of us from using that release.

In a message-based, multi-language, distributed architecture like ROS, I believe the best solution is to treat Python 2 and Python 3 as separate languages and support them both. That makes it possible to break the circular dependencies by allowing both dialects to coexist as nodes in a single ROS graph. Then we can all move forward.

When I said: "I still see no signs of movement in that direction, or even recognition that it is a problem", that is what I had in mind. I am not trying to insult anyone. Maybe you do recognize this problem and already have a solution in mind. If so, I would like to know what it is.

@dirk-thomas
Copy link
Member Author

@jack-oquin Regarding the __hash__ methods: please feel free to fill a ticket to track that and provide more detailed information. I don't yet understand fully what the issue with the current code is and in which cases it would fail. And example / test where it currently fails with Python 3 would be great. The response of python -3 is not necessary a good indicator since it only lists code which can not be easily converted by the 2to3 script (which is potentially not relevant).

@jack-oquin
Copy link
Member

So, you don't think suppressing python -3 warnings is important?

@dirk-thomas
Copy link
Member Author

I will happily fix any bug and clean up any inconsistent code. What I was referring to before was that python -3 has a very specific goal: to indicate potential problems which might occur when running 2to3. Since that will never happen the output is not mandatory. If one warning of that invocation is actually a bug or bad/inconistent code then it will for sure be addressed.

cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
This reverts commit 125aee3.

This got fixed upstream, so perception_pcl (ros-perception/perception_pcl@8b32dcd) did remove this hack too.
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.

4 participants