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

[17.09] Pulsar remote metadata fixes #4622

Merged

Conversation

Projects
None yet
5 participants
@natefoo
Copy link
Member

commented Sep 15, 2017

Because Pulsar cannot use a subshell for the tool command (see galaxyproject/pulsar#137 for details), we must provide a way to properly set up the Galaxy virtutalenv and lib after the tool command is run and before set_metadata.py in order to use remote metadata. To do this, I've slightly overhauled and removed some duplication in the job script.

Note: $VIRTUAL_ENV being set will no longer prevent $GALAXY_VIRTUAL_ENV from being used. This is because I can't think of a way that, under Galaxy, $GALAXY_VIRTUAL_ENV and $VIRTUAL_ENV would not be the same thing, if $VIRTUAL_ENV was set at all (it is technically possible to run from outside a virtualenv, and in this case, $GALAXY_VIRTUAL_ENV will be None). Further, the $VIRTUAL_ENV handling wouldn't have worked for Pulsar because under Pulsar it was set to Pulsar's virtualenv, which prevented Galaxy's virtualenv from being activated.

I am not sure if this was the intent, but while $PRESERVE_GALAXY_ENVIRONMENT prevents $PYTHONPATH from including Galaxy's lib directory, the first python on $PATH will still be the one Galaxy started with (and honestly, what else would it be?). The conditional of $PRESERVE_GALAXY_ENVIRONMENT for the virtualenv doesn't really add anything there other than not reactivating the already activated virtualenv.

If I am missing something about the two paragraphs above, please let me know.

This also adds $PRESERVE_GALAXY_ENVIRONMENT support for Pulsar.

Because these changes are really only for remote metadata and I'm probably the only person using it, I added some conditional feature checking to the remote Pulsar version check so that people wouldn't be forced to upgrade their Pulsar servers just because of this change.

Tests will not pass until we release a new version of pulsar-galaxy-lib.

-a "`command -v python`" != "$GALAXY_VIRTUAL_ENV/bin/python" ]; then
. "$GALAXY_VIRTUAL_ENV/bin/activate"
fi
}

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 15, 2017

Member

It feels like this should come after $headers? Maybe $integrity_injection also?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 15, 2017

Member

I'd say between $headers (which may contain directives for the cluster scheduler) and $integrity_injection.

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 15, 2017

Author Member

It executes after both, is there a problem with it being defined before either?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 15, 2017

Member

I fear scheduler directives may have to come first.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

The conditional of $PRESERVE_GALAXY_ENVIRONMENT for the virtualenv doesn't really add anything there other than not reactivating the already activated virtualenv.

But it doesn't seem like you removed it then - or am I misreading this?

$env_setup_commands
GALAXY_VIRTUAL_ENV="$galaxy_virtual_env"
if [ "$GALAXY_VIRTUAL_ENV" != "None" -a -z "$VIRTUAL_ENV" \
-a -f "$GALAXY_VIRTUAL_ENV/bin/activate" -a "$PRESERVE_GALAXY_ENVIRONMENT" = "True" ]; then

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 15, 2017

Member

This was a bug right? A very serious bug - there shouldn't have been a check on PRESERVE_GALAXY_ENVIRONMENT here.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 15, 2017

Member

There's still a check on PRESERVE_GALAXY_ENVIRONMENT at the moment.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

It would be nice to compare a job script before and after these changes, it's bit hard to figure it out from the code changes.

$headers
$integrity_injection
$slots_statement
export GALAXY_SLOTS
GALAXY_VIRTUAL_ENV="$galaxy_virtual_env"

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 15, 2017

Member

By moving this line here it will be possible to specify the Galaxy virtualenv on the cluster nodes inside the script specified by environment_setup_file or by adding <env id="GALAXY_VIRTUAL_ENV">/PATH/TO/VIRTUALENV</env> instead of having to use <env file="/PATH/TO/VIRTUALENV/bin/activate" /> . Thanks @natefoo!

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 15, 2017

Member

Yeah - that is huge - it means someday we can conditionally not activate it (e.g. for profile>=18.01 tools). We should probably scan the env's for jobs for file=".../bin/activate" and issue a warning in Galaxy's log to convert this.

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 15, 2017

Author Member

I totally meant to do that.

Worth noting that without additional changes, if you were to force this to GALAXY_VIRTUAL_ENV="None" via the mechanisms @nsoranzo described, jobs would most likely fail to run set_metadata.py.

In an earlier draft of this work I had saved this value with _GALAXY_VIRTUAL_ENV="$GALAXY_VIRTUAL_ENV" and added handling to put the original value back in for the metadata call. I could add that back in if you'd like.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 15, 2017

Member

That is where I am sitting I think.

Update: I placed this comment in the wrong place - it should have been in response to the now hidden comment about consensus.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 15, 2017

Member

Not necessary IMO.

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 16, 2017

Author Member

Oh... well, it's there. Should I take it back out?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Sep 16, 2017

Member

It seems to unnecessarily complicate the code, but no strong feelings.

$env_setup_commands
if [ "$GALAXY_VIRTUAL_ENV" != "None" -a "$_use_framework_galaxy" = "True" \
-a -f "$GALAXY_VIRTUAL_ENV/bin/activate" \
-a "`command -v python`" != "$GALAXY_VIRTUAL_ENV/bin/python" ]; then

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 15, 2017

Author Member

@jmchilton: Was the consensus from Gitter that we should drop the -a "$_use_framework_galaxy" = "True" from this expression? Everything else is okay here?

This comment has been minimized.

Copy link
@nsoranzo

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 16, 2017

Member

I commented elsewhere (now hidden) but yet - I'd remove that check.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2017

@nsoranzo: here's an example:

#!/bin/bash

_galaxy_setup_environment() {
    local _use_framework_galaxy="$1"
    if [ "$GALAXY_LIB" != "None" -a "$_use_framework_galaxy" = "True" ]; then
        if [ -n "$PYTHONPATH" ]; then
            PYTHONPATH="$GALAXY_LIB:$PYTHONPATH"
        else
            PYTHONPATH="$GALAXY_LIB"
        fi
        export PYTHONPATH
    fi
    
    if [ "$GALAXY_VIRTUAL_ENV" != "None" -a  "$_use_framework_galaxy" = "True" \
         -a -f "$GALAXY_VIRTUAL_ENV/bin/activate" \
         -a "`command -v python`" != "$GALAXY_VIRTUAL_ENV/bin/python" ]; then
        . "$GALAXY_VIRTUAL_ENV/bin/activate"
    fi
}



# The following block can be used by the job system
# to ensure this script is runnable before actually attempting
# to run it.
if [ -n "$ABC_TEST_JOB_SCRIPT_INTEGRITY_XYZ" ]; then
    exit 42
fi

GALAXY_SLOTS="1"; export GALAXY_SLOTS;
export GALAXY_SLOTS
GALAXY_VIRTUAL_ENV="/home/nate/work/galaxy/.venv"
PRESERVE_GALAXY_ENVIRONMENT="True"
GALAXY_LIB="/home/nate/work/galaxy/lib"
_galaxy_setup_environment "$PRESERVE_GALAXY_ENVIRONMENT"
GALAXY_PYTHON=`command -v python`
echo "$GALAXY_SLOTS" > '/home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_galaxy_slots' 
date +"%s" > /home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_epoch_start
cd /home/nate/work/galaxy/database/jobs_directory/000/0
rm -rf working; mkdir -p working; cd working; /home/nate/work/galaxy/database/jobs_directory/000/0/tool_script.sh; return_code=$?; cd '/home/nate/work/galaxy/database/jobs_directory/000/0'; 
_galaxy_setup_environment True
python "/home/nate/work/galaxy/database/jobs_directory/000/0/set_metadata.py" "/home/nate/work/galaxy/database/jobs_directory/000/0/registry.xml"
"/home/nate/work/galaxy/database/jobs_directory/000/0/working/galaxy.json" "/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_in_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_kwds_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_out_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_results_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/files/000/dataset_0.dat,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_override_HistoryDatasetAssociation_0" 5242880; sh -c "exit $return_code"
echo $? > /home/nate/work/galaxy/database/jobs_directory/000/0/galaxy_0.ec
date +"%s" > /home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_epoch_end

And a diff from an old one:

--- old_job_script.sh	2017-09-15 15:06:47.755263076 -0400
+++ new_job_script.sh	2017-09-15 15:11:29.111591829 -0400
@@ -1,5 +1,23 @@
 #!/bin/bash
 
+_galaxy_setup_environment() {
+    local _use_framework_galaxy="$1"
+    if [ "$GALAXY_LIB" != "None" -a "$_use_framework_galaxy" = "True" ]; then
+        if [ -n "$PYTHONPATH" ]; then
+            PYTHONPATH="$GALAXY_LIB:$PYTHONPATH"
+        else
+            PYTHONPATH="$GALAXY_LIB"
+        fi
+        export PYTHONPATH
+    fi
+    
+    if [ "$GALAXY_VIRTUAL_ENV" != "None" -a  "$_use_framework_galaxy" = "True" \
+         -a -f "$GALAXY_VIRTUAL_ENV/bin/activate" \
+         -a "`command -v python`" != "$GALAXY_VIRTUAL_ENV/bin/python" ]; then
+        . "$GALAXY_VIRTUAL_ENV/bin/activate"
+    fi
+}
+
 
 
 # The following block can be used by the job system
@@ -11,40 +29,16 @@
 
 GALAXY_SLOTS="1"; export GALAXY_SLOTS;
 export GALAXY_SLOTS
-echo "python is $(command -v python)"
-PRESERVE_GALAXY_ENVIRONMENT="False"
-GALAXY_LIB="/home/nate/work/galaxy/lib"
-if [ "$GALAXY_LIB" != "None" -a "$PRESERVE_GALAXY_ENVIRONMENT" = "True" ]; then
-    if [ -n "$PYTHONPATH" ]; then
-        PYTHONPATH="$GALAXY_LIB:$PYTHONPATH"
-    else
-        PYTHONPATH="$GALAXY_LIB"
-    fi
-    export PYTHONPATH
-fi
-
 GALAXY_VIRTUAL_ENV="/home/nate/work/galaxy/.venv"
-echo "python is $(command -v python)"
-if [ "$GALAXY_VIRTUAL_ENV" != "None" -a -z "$VIRTUAL_ENV" \
-     -a -f "$GALAXY_VIRTUAL_ENV/bin/activate" -a "$PRESERVE_GALAXY_ENVIRONMENT" = "True" ]; then
-    . "$GALAXY_VIRTUAL_ENV/bin/activate"
-fi
+PRESERVE_GALAXY_ENVIRONMENT="True"
+GALAXY_LIB="/home/nate/work/galaxy/lib"
+_galaxy_setup_environment "$PRESERVE_GALAXY_ENVIRONMENT"
+GALAXY_PYTHON=`command -v python`
 echo "$GALAXY_SLOTS" > '/home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_galaxy_slots' 
 date +"%s" > /home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_epoch_start
 cd /home/nate/work/galaxy/database/jobs_directory/000/0
 rm -rf working; mkdir -p working; cd working; /home/nate/work/galaxy/database/jobs_directory/000/0/tool_script.sh; return_code=$?; cd '/home/nate/work/galaxy/database/jobs_directory/000/0'; 
-if [ "$GALAXY_LIB" != "None" ]; then
-    if [ -n "$PYTHONPATH" ]; then
-        PYTHONPATH="$GALAXY_LIB:$PYTHONPATH"
-    else
-        PYTHONPATH="$GALAXY_LIB"
-    fi
-    export PYTHONPATH
-fi
-if [ "$GALAXY_VIRTUAL_ENV" != "None" -a -z "$VIRTUAL_ENV"      -a -f "$GALAXY_VIRTUAL_ENV/bin/activate" ]; then
-    . "$GALAXY_VIRTUAL_ENV/bin/activate"
-fi
-GALAXY_PYTHON=`command -v python`
+_galaxy_setup_environment True
 python "/home/nate/work/galaxy/database/jobs_directory/000/0/set_metadata.py" "/home/nate/work/galaxy/database/jobs_directory/000/0/registry.xml" "/home/nate/work/galaxy/database/jobs_directory/000/0/working/galaxy.json" "/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_in_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_kwds_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_out_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_results_HistoryDatasetAssociation_0,/home/nate/work/galaxy/database/files/000/dataset_0.dat,/home/nate/work/galaxy/database/jobs_directory/000/0/metadata_override_HistoryDatasetAssociation_0" 5242880; sh -c "exit $return_code"
 echo $? > /home/nate/work/galaxy/database/jobs_directory/000/0/galaxy_0.ec
 date +"%s" > /home/nate/work/galaxy/database/jobs_directory/000/0/__instrument_core_epoch_end
@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2017

I think I implemented the suggestions given here, let me know if anything is amiss. If it looks good, I'll push the updated template to the Pulsar PR. @jmchilton don't merge and release that PR until it's in sync with the "final" version of this one.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

@natefoo Apropos this - I had to hack up my CWL branch to restore PATHs that are rewritten by tools. common-workflow-language@0189c34. So that is one answer to your question about how you might get the wrong Python.

We need to I think separate environment variable updates that job destination describes from those that tools (and/or tool dependencies) describe and ensure the tool describing ones are not present when the metadata is collected for Galaxy. If you are ever in their hacking and have some ideas - that would be great. It is also becoming very clear to me that job_script and command builder need to somehow be merged together I think or at least move the metadata collection out of the command-builder's prevue and into the job script's.

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

if this is considered a bugfix it needs to be re-targeted to the release_17.09 since we have branched

@natefoo natefoo modified the milestones: 17.09, 18.01 Sep 20, 2017

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

@jmchilton That's exactly what this work is fixing, fixing $PATH and $PYTHONPATH for set_metadata.py if the tool has messed them up. However, unless you start Galaxy outside of a virtualenv, $VIRTUAL_ENV will be set and $GALAXY_VIRTUAL_ENV will always point at it.

Your CWL hack accomplishes the same thing, but it seems a little cleaner and more explicit than my change, should I switch to that? There's one thing I'm not sure on though: If $PRESERVE_GALAXY_ENVIRONMENT is not True, then $PYTHONPATH will not get $GALAXY_LIB before the tool runs, and $_GALAXY_PYTHONPATH will not include $GALAXY_LIB. However, I assume you have this working, so I am not sure what I'm missing.

@martenson Will do.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

@mvdbeek hopefully 7c86cd4 doesn't mess up your work in #4600. I can't find any other caller that sets the datatypes_config metadata_kwd.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

It looks OK to me, pulsar was something that I didn't quite know how to deal with.

@natefoo natefoo changed the base branch from dev to release_17.09 Sep 21, 2017

@natefoo natefoo changed the title Pulsar remote metadata fixes [17.09] Pulsar remote metadata fixes Sep 21, 2017

@natefoo natefoo modified the milestones: 18.01, 17.09 Sep 21, 2017

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

Rebased to 17.09.

@jmchilton any thoughts on the script changes?

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

Your CWL hack accomplishes the same thing, but it seems a little cleaner and more explicit than my change, should I switch to that?

We aren't distinguishing between tool environment variable declarations and job env ones right? They all get dumped into a hash together. If we somehow revert the tool definitions we also revert the job handlers specified definitions. When we go in refactor metadata collection out of the command builder and into the job script stuff we need to segregate tool environment declarations and job handler ones at the same time I think - so that we can revert the tool ones and preserve the job handler ones. As a short term hack for 17.09 I don't know which approach is better - maybe your existing one since it doesn't revert job handler stuff?

There's one thing I'm not sure on though: If $PRESERVE_GALAXY_ENVIRONMENT is not True, then $PYTHONPATH will not get $GALAXY_LIB before the tool runs, and $_GALAXY_PYTHONPATH will not include $GALAXY_LIB. However, I assume you have this working, so I am not sure what I'm missing.

No - I think I am still just recalculating it twice. I think that hack is fixing the PATH problem I encountered and doing squat for PYTHONPATH to be honest.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

Ok, thanks. Guess we'll stick with it as is.

@natefoo

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

We're gonna need #4674 to be able to run tests.

@natefoo natefoo force-pushed the natefoo:pulsar-remote-metadata-fixes branch from 841a4b6 to db5185c Sep 21, 2017

@jmchilton jmchilton merged commit d1be9e6 into galaxyproject:release_17.09 Sep 22, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.