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

Allow selection of python version when starting managed Galaxy #874

Merged
merged 20 commits into from
Oct 18, 2018

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Sep 29, 2018

Make sure python 3 is on PATH and enjoy planemo {test, serve} --galaxy_branch release_18.09 --galaxy_python_version 3.

@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch from 302b226 to f1b9c9d Compare September 29, 2018 19:43
@mvdbeek
Copy link
Member Author

mvdbeek commented Sep 29, 2018

Hmm, this is Paste failing now due to deleting from a dict while iterating over it. The fix is simple and I've done it locally, but Paste looks pretty much unmaintained.

@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch 9 times, most recently from 4e963af to 7b9a4b9 Compare September 29, 2018 21:45
@mvdbeek mvdbeek closed this Sep 29, 2018
@mvdbeek mvdbeek reopened this Sep 29, 2018
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch 7 times, most recently from 6f4d748 to a4d78b9 Compare September 29, 2018 22:04
@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 1, 2018

Hmm, so for some reason when installing dependencies by running common_startup.sh we get the wrong shebang (it points to the python in the virtualenv in which planemo was installed), which is a problem for the gunicorn entrypoint. I haven't figured out what's going on there, seems very strange.

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 1, 2018

So I'm having __PYVENV_LAUNCHER__ set in my python 3 virtualenv. This gets propagated to new commands lauched by planemo because in galaxy-lib we create a new env with os.environ.copy and update it. Just setting env['__PYVENV_LAUNCHER__'] = '' seems to solve the issue. Seems this problem might occur more frequently in the future based on pypa/virtualenv#845 (comment)

mvdbeek added a commit to mvdbeek/planemo that referenced this pull request Oct 1, 2018
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch 2 times, most recently from d3b676e to 2394309 Compare October 1, 2018 11:42
mvdbeek added a commit to mvdbeek/planemo that referenced this pull request Oct 1, 2018
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch from 2394309 to 35eb2c1 Compare October 1, 2018 11:46
mvdbeek added a commit to mvdbeek/planemo that referenced this pull request Oct 1, 2018
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch 2 times, most recently from 48d9807 to fafb98b Compare October 1, 2018 21:41
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch from fafb98b to c784925 Compare October 6, 2018 08:48
@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 6, 2018

yay, all green!

.travis.yml Show resolved Hide resolved
@mvdbeek mvdbeek force-pushed the allow_python_version_selection branch from 05f66a8 to dbfec70 Compare October 16, 2018 13:01
@nsoranzo
Copy link
Member

So I'm having __PYVENV_LAUNCHER__ set in my python 3 virtualenv. This gets propagated to new commands lauched by planemo because in galaxy-lib we create a new env with os.environ.copy and update it. Just setting env['__PYVENV_LAUNCHER__'] = '' seems to solve the issue. Seems this problem might occur more frequently in the future based on pypa/virtualenv#845 (comment)

@mvdbeek A bugfix for this has been proposed in python/cpython#9516

@@ -400,7 +401,7 @@ def config_join(*args):
template_args = dict(
port=port,
host=kwds.get("host", "127.0.0.1"),
server_name=server_name,
server_name=server_name if float(kwds.get('galaxy_python_version', DEFAULT_PYTHON_VERSION)) < 3 else 'main', # gunicorn needs main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

            server_name=server_name if parse_version(kwds.get('galaxy_python_version', DEFAULT_PYTHON_VERSION)) < parse_version('3') else 'main',  # gunicorn needs main

after adding from pkg_resources import parse_version at the top.

@@ -941,6 +942,14 @@ def startup_command(self, ctx, **kwds):
run_script += " --server-name %s" % shlex_quote(self.server_name)
server_ini = os.path.join(self.config_directory, "galaxy.ini")
self.env["GALAXY_CONFIG_FILE"] = server_ini
if float(kwds.get('galaxy_python_version', DEFAULT_PYTHON_VERSION)) >= 3:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if float(kwds.get('galaxy_python_version', DEFAULT_PYTHON_VERSION)) >= 3:
if parse_version(kwds.get('galaxy_python_version', DEFAULT_PYTHON_VERSION)) >= parse_version('3'):

@@ -1240,6 +1254,7 @@ def _install_with_command(ctx, config_directory, command, env, kwds):
else:
pip_install_command = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that, since commit 9867e56, pip_installs and pip_install_command could be removed, if desired.

planemo/io.py Outdated
@@ -182,6 +182,7 @@ def kill_pid_file(pid_file):

pid = int(open(pid_file, "r").read())
kill_posix(pid)
os.unlink(pid_file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in a try/except Exception: pass, since the killed process can remove the pid file before exiting, in which case os.remove would raise OSError or FileNotFoundError.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks!

@nsoranzo nsoranzo merged commit 7ca2b40 into galaxyproject:master Oct 18, 2018
@nsoranzo
Copy link
Member

Thanks @mvdbeek!

Should we enable this for tools-iuc testing? 😈

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 18, 2018

Sure! We can start with a blacklist and work our way down like we did with condafying tools.

nsoranzo added a commit to nsoranzo/planemo that referenced this pull request Nov 23, 2018
Otherwise the `pid_file` is set to the wrong filename, preventing
`LocalGalaxyConfig.kill()` from working when running Galaxy under Python 3.

Follow-up to galaxyproject#874 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants