search

pepkit / divvy

Standardized computing resource configuration
http://divvy.databio.org
BSD 2-Clause "Simplified" License
4 stars 2 forks source link

compute_env_var returns list, not str #22

Closed vreuter closed 5 years ago

vreuter commented 5 years ago

Previously we returned "PEPENV" and now it feels like we should return "DIVCFG". The name of the property (compute_env_var) suggests a single value rather than a collection, and in the docstring we specify a str result. Why do we return a list?

stolarczyk commented 5 years ago

It is a list since we needed two env var to be checked to preserve backwards compatibility. See: https://github.com/pepkit/peppy/commit/45b9e70558c3e0cdd28f5edb78cac520cd8f882e#r32414952

stolarczyk commented 5 years ago

I think it will get back to str when we make the peppy v1.0 release and switch to DIVCFG entirely in divvy.

vreuter commented 5 years ago

OK thanks, that totally makes sense. It means for the time being that we must redeclare COMPUTE_SETTINGS_VARNAME in the peppy package file (https://github.com/pepkit/peppy/commit/a0324041b9c2ce5e0ea8e917b2543f66b8852d43) but yeah then we can remove and depend on the single one from divvy like you said.

So that I remember to remove the redeclaration I introduced to make looper --version run with old looper but new peppy, there's https://github.com/pepkit/peppy/issues/272. i just need the redeclaration for now since the help text for looper (at least the old version) uses a getenv call based on that constant (https://github.com/pepkit/looper/blob/ee33a507d93f5ff5f52381e61b379b32314d7b8c/looper/looper.py#L122-L125)

stolarczyk commented 5 years ago

i just need the redeclaration for now since the help text for looper (at least the old version) uses a getenv call based on that constant

Sure. The getenv call in the help text is changed to None in looper on argparser_caravel and handled by divvy at the Project object creation stage.

And that's why it works there