databio / bulker

Manager for multi-container computing environments
https://bulker.io
BSD 2-Clause "Simplified" License
24 stars 2 forks source link

Don't use $HOME, hard-code it #66

Open nsheff opened 3 years ago

nsheff commented 3 years ago

CWL rewrites $HOME for its runs, so it doesn't work with bulker shims, which by default are mounting $HOME as an env var...

I see no real value in keeping the variable in the bulker config, so when the config is initiated, by default we should probably simply resolve the environment variable at the config build time instead of maintaining the environment variable clear through to the containerized executable scripts.

nsheff commented 3 years ago

The bulker config is a YacAttMap, which is a PathExAttMap -- so in theory it should be expanding those env vars... but it doesn't

Writing the config uses the .to_yaml() method on the YacAttMap, which is not populating $HOME.

import attmap
In [3]: attmap.PathExAttMap({"x": "$HOME"})
Out[3]: 
PathExAttMap
x: $HOME

In [4]: am = attmap.PathExAttMap({"x": "$HOME"})

In [7]: am.to_yaml()
Out[7]: 'x: $HOME\n'

In [8]: am.to_dict()
Out[8]: {'x': '$HOME'}

@stolarczyk what's the recommended way to make this populate env vars automatically since it doesn't do it by default for .to_yaml() -- it does it by default only for item or attribute access

In [5]: am.x
Out[5]: '/home/nsheff'
In [9]: am["x"]
Out[9]: '/home/nsheff'

Do we need a to_yaml(expand=True) option?

stolarczyk commented 3 years ago

The bulker config is a YacAttMap, which is a PathExAttMap -- so in theory it should be expanding those env vars... but it doesn't

isn't the default use case to keep the env vars not expanded, to maintain the configs portability?

it doesn't do it by default for .to_yaml() -- it does it by default only for item or attribute access

don't want to speak for @vreuter, but to my understanding, that was exactly the idea behind this class

what's the recommended way to make this populate env vars automatically

I don't think there is a dedicated method that does that at the moment. Working with what we have now, I'd do this:

with open(path, 'w') as y:
    yaml.dump(x.to_dict(expand=True), y, default_flow_style=False)

Do we need a to_yaml(expand=True) option?

that would be useful

nsheff commented 3 years ago

This all makes sense, and you're right, I think it's the way it should be. So, what I'm looking for is to_yaml(expand=True). Raised an issue there.