search

nils-braun / b2luigi

Task scheduling and batch running for basf2 jobs made simple
GNU General Public License v3.0
17 stars 11 forks source link

import get_setting for lsf tasks #83

Closed MarcelHoh closed 3 years ago

MarcelHoh commented 3 years ago

Hi, when we swtiched to get_setting for the queue and job_name parameter we did not add an import to use get_setting.

There is an extra problem in that for the queue and job_name parameter the default value is set to None. However get_setting internally takes None to mean the setting is not defined and throws a ValueError. Should this be removed to allow for the default value to be None?

MarcelHoh commented 3 years ago

Just saw both issues are already fixed in main! That was silly.

meliache commented 3 years ago

@MarcelHoh Still thanks for reporting. Last week I was a bit impatient with PR's because I wanted to merge most open PR's before publishing a new release to make the latest version available on PyPi, i.e. so that you can just do pip3 install --upgrade b2luigi instead of having to install the development version, e.g. via pip3 install --upgrade "git+https://github.com/nils-braun/b2luigi".

I had somehow assumed you had already tested the new settings code in your PR, but really, I should have tested it myself. I saw the error when implementing the job_name setting for htcondor, where the same error appeared. Actually, I noticed the missing import right after opening the file in emacs, because my syntax checker underlined that line in red (but any IDE would have done so). This just shows that we should add static syntax checking (e.g. flake8) to our github actions.

Anyway, I fixed it in #81 if I had the time I would have made you reviewer, but somehow I really wanted to get done with the new release on the weekend. Btw, thanks to you I just noticed that I have mistakenly not included that PR in v0.6.0, so I just published a new patch release v0.6.1

Tipp: You can use the watch button on github to get email notifications for new releases or PR's and Issues. You can customize via the dropdown menu what events you'll get notifications for.