[16.01] Allow override of job shell (for conda resolver). by jmchilton · Pull Request #1473 · galaxyproject/galaxy

jmchilton · 2016-01-11T20:50:26Z

common_startup assumes bashisms anyway. I'm making more things configurable in case someday the BSD user wants to use Galaxy - the default shell for jobs can be configured at the galaxy or job destination level now.

nsoranzo · 2016-01-12T11:47:17Z

lib/galaxy/jobs/__init__.py

s/getattr(self.app.config, 'default_job_shell', DEFAULT_JOB_SHELL)/self.app.config.get('default_job_shell', DEFAULT_JOB_SHELL)/

def get( self, key, default ): return self.config_dict.get( key, default )

I strongly prefer my version. I want the processed value on the config object itself. Also, I like my access pattern assumes less about config in terms of a required interface.

I think that if they are not equivalent in this case, then that should be fixed. Isn't it the whole point of an interface to hide where the property is actually stored?

In this particular case because the property is a string and the config property matches the name in galaxy.ini - these two are likely equivalent. But for booleans, numbers, etc... the config object attributes and not the dictionary contains the processed value - it is however for this reason that the canonical source of the information should be assumed to be the object attribute.

Most places in the code that I have seen access config options as attributes and via .get() - I don't think this is a problem and I don't think we promise get() does anything other than access the raw values in a dictionary like way. Instead of modifying get, I would rather see code that accesses config options this way access them via attributes instead.

nsoranzo · 2016-01-12T12:37:38Z

It seems to me that scripts/common_startup.sh does not contain bashisms, and seems to work after s|/bin/bash|/bin/sh| under Ubuntu dash.

jmchilton · 2016-01-12T14:38:19Z

@nsoranzo I'll rebase with a new comment - that was information @natefoo had passed along which I guess is wrong (... or is it possible dash implements some bashisms on top of sh)?

jmchilton · 2016-01-12T15:01:39Z

Updated the comment.

I had to modify one test tool because this breaks backward compatiblity on how the literal \t characters are handled jmchilton@e8b6b9e. dash will replace these with a tab in strings - bash requires more work.

We could alternatively replace instances of \t in command with tab. Before doing that, I'll see if I can find a bash option to fix this.

Update: bash --posix doesn't fix this.

jmchilton · 2016-01-12T15:23:43Z

@nsoranzo You care a good deal about backward compatibility - what do you think about the fact that we need to make the change. Are you okay with it, since the original shell foo I was using wasn't very portable?

Update: I'll switch to the printf version.

nsoranzo · 2016-01-12T15:49:14Z

@jmchilton Isn't that just in a test?

jmchilton · 2016-01-12T16:20:24Z

I mean the test is indicating the PR is changing the behavior of Galaxy tools in a backward incompatible way. Is it okay to change Galaxy because the tool was poorly written and not very portable.

nsoranzo · 2016-01-12T16:48:15Z

Probably the default shell should remain '/bin/sh' since conda is not in real use yet and conda may be fixed to support other shells in the mean time.

jmchilton · 2016-01-12T21:41:11Z

Ping @bgruening - do you thing we should delay the default switch one release or fix conda now?

bgruening · 2016-01-12T22:08:58Z

Sorry was travelling. We ship conda with this release so I would welcome the switch, but I can understand the concerns from @nsoranzo.

@nsoranzo do you have any insider news about anaconda to support /bin/sh in the near future?

Following suggestion: We leave bash for this release but deprecate it and announce the switch for the next release. Hopefully a few galaxy-admins will test this during this release cycle and provide feedback.
If we do this we should include a small text to the conda_auto_install config entry and state that the shell should be set to /bin/bash.

I don't think this switch is will case much trouble as most of us already using bash but we probably don't know. This is a SL6.6:

[galaxy@cn066 ~]$ ls -l /bin/sh 
lrwxrwxrwx 1 root root 4  4. Feb 2015  /bin/sh -> bash
[galaxy@cn066 ~]$ uname -a
Linux cn066.bi.uni-freiburg.de 2.6.32-573.12.1.el6.x86_64 #1 SMP Tue Dec 15 08:24:23 CST 2015 x86_64 x86_64 x86_64 GNU/Linux
[galaxy@cn066 ~]$ cat /etc/lsb-release
LSB_VERSION=base-4.0-amd64:base-4.0-noarch:core-4.0-amd64:core-4.0-noarch:graphics-4.0-amd64:graphics-4.0-noarch:printing-4.0-amd64:printing-4.0-noarch

jmchilton · 2016-01-12T23:58:08Z

Okay - switched the default back but this fix is still required for conda to work so I think this PR should still be merged and I would propose docker-galaxy-stable and usegalaxy.org switch the default to /bin/bash for this release cycle.

bgruening · 2016-01-13T00:05:02Z

Indeed! I will make this default in Docker!

nsoranzo · 2016-01-13T02:55:59Z

config/galaxy.ini.sample

s/switched/not switched/

Fixed, sorry about that.

nsoranzo · 2016-01-13T10:07:11Z

@bgruening No, I don't have any news, actually I've never used conda!

nsoranzo · 2016-01-13T20:11:41Z

👍

Use user's conda environment for planemo - this way if conda_init, conda_install will setup subsequent test and serve calls. Also use /bin/bash as the default job shell - requires merging galaxyproject/galaxy#1473.

I'm making more things configurable in case someday the BSD user wants to use Galaxy - the default shell for jobs can be configured at the galaxy or job destination level now.

dash will convert the literal characters \t to a tab, bash doesn't seem to do this. Use portable printf as suggested by nicola.

bgruening · 2016-01-14T10:58:37Z

@galaxybot test this

bgruening · 2016-01-14T10:59:16Z

@erasche the assigned milestone is wrong
@jmchilton we need a rebase here

Thanks!

jmchilton · 2016-01-14T11:44:11Z

@bgruening It has been rebased, thanks for the heads up.

[16.01] Allow override of job shell (for conda resolver).

bgruening · 2016-01-14T12:36:08Z

Thanks @jmchilton and @nsoranzo!

jmchilton · 2016-01-14T12:36:13Z

Thanks for the detailed review @nsoranzo and the merge @bgruening.

natefoo · 2016-01-14T16:09:47Z

Could you create an issue on usegalaxy-playbook noting the need for a config change on Test/Main please?

nsoranzo reviewed Jan 12, 2016
View reviewed changes

jmchilton force-pushed the job_shell branch from 01b2be3 to e8b6b9e Compare January 12, 2016 14:52

jmchilton force-pushed the job_shell branch from e8b6b9e to b89befa Compare January 12, 2016 23:56

jmchilton changed the title ~~[16.01] Switch to bash as default shell (for conda resolver).~~ [16.01] Allow override of job shell (for conda resolver). Jan 12, 2016

nsoranzo reviewed Jan 13, 2016
View reviewed changes

jmchilton force-pushed the job_shell branch from b89befa to cddb3d5 Compare January 13, 2016 19:20

nsoranzo added kind/enhancement status/review area/framework labels Jan 13, 2016

jmchilton force-pushed the job_shell branch 2 times, most recently from 7533320 to 8a058cb Compare January 13, 2016 20:08

galaxybot added the status/review label Jan 13, 2016

galaxybot added this to the 16.04 milestone Jan 13, 2016

galaxybot added the triage label Jan 13, 2016

hexylena removed the triage label Jan 13, 2016

jmchilton added 3 commits January 14, 2016 06:17

Switch to bash as default shell (for conda resolver).

3e47aa2

I'm making more things configurable in case someday the BSD user wants to use Galaxy - the default shell for jobs can be configured at the galaxy or job destination level now.

Fix tab problem in test tool.

e696382

dash will convert the literal characters \t to a tab, bash doesn't seem to do this. Use portable printf as suggested by nicola.

Switch default job shell back to /bin/sh for 16.01.

602dc7f

bgruening modified the milestones: 16.01, 16.04 Jan 14, 2016

jmchilton force-pushed the job_shell branch from 8a058cb to 602dc7f Compare January 14, 2016 11:17

bgruening added a commit that referenced this pull request Jan 14, 2016

Merge pull request #1473 from jmchilton/job_shell

be4ddb2

[16.01] Allow override of job shell (for conda resolver).

bgruening merged commit be4ddb2 into galaxyproject:release_16.01 Jan 14, 2016

jmchilton deleted the job_shell branch January 14, 2016 12:36

jmchilton added the kind/bug label Jan 14, 2016

Conversation

jmchilton commented Jan 11, 2016

Uh oh!

nsoranzo Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

jmchilton Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

nsoranzo Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

jmchilton Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

nsoranzo commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

nsoranzo commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

nsoranzo commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

bgruening commented Jan 12, 2016

Uh oh!

jmchilton commented Jan 12, 2016

Uh oh!

bgruening commented Jan 13, 2016

Uh oh!

nsoranzo Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

jmchilton Jan 13, 2016

Choose a reason for hiding this comment

Uh oh!

nsoranzo commented Jan 13, 2016

Uh oh!

nsoranzo commented Jan 13, 2016

Uh oh!

bgruening commented Jan 14, 2016

Uh oh!

bgruening commented Jan 14, 2016

Uh oh!

jmchilton commented Jan 14, 2016

Uh oh!

bgruening commented Jan 14, 2016

Uh oh!

jmchilton commented Jan 14, 2016

Uh oh!

natefoo commented Jan 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants