[Bug] Silent crashes with invalid database label names

[lsago]

Describe the bug
Using a "-" for a database label results in a crashed algorithm. It can be a bit confusing to troubleshoot.

To Reproduce
Steps to reproduce the behavior:

1. Add an entry to the database like:

   - type: 'csv'
     uri: '/path/to/letters-data.csv'
     label: 'letters-dash'

2. Create a task using this label, like:

   average_task = client.task.create(
                                 ...
                                 image="harbor2.vantage6.ai/demo/average",
                                 databases=[
                                     {'label': 'letters-dash'}
                                 ])

3. On the node, logs will show up:

   info > Reading data from database
   error > Error encountered while calling partial_average: 'LETTERS-DASH_DATABASE_URI'
   error > None
   Traceback (most recent call last):
     File "/app/vantage6-algorithm-tools/vantage6/algorithm/tools/wrap.py", line 110, in _run_algorithm_method
       result = method(*args, **kwargs)
     File "/app/vantage6-algorithm-tools/vantage6/algorithm/tools/decorators.py", line 152, in decorator
       data_ = _get_data_from_label(label)
     File "/app/vantage6-algorithm-tools/vantage6/algorithm/tools/decorators.py", line 188, in _get_data_from_label
       database_uri = os.environ[f"{label.upper()}_DATABASE_URI"]
     File "/usr/local/lib/python3.10/os.py", line 680, in __getitem__
       raise KeyError(key) from None
   KeyError: 'LETTERS-DASH_DATABASE_URI'
   

Expected behavior
It's a very minor issue, but perhaps it is worth a warning or error when including non-valid characters in database labels. Either at client side or a node side. Or both..

Details

• OS: Linux
• Version: 4.0.2
• Docker deamon version: 24.0.6

Once #154 is tackled, it seems very likely that there won't be a need to create these environment variables with the label in their name, so this shouldn't be an issue after that.

[lsago]

Cause (probably)

Algorithm images are usually built on top of the algorithm wrapper. This is using python:3.10-slim-bullseye as base image at the moment, which as a Debian based image has dash as its /bin/sh. Docker will use that shell when using the CMD instruction in "shell form" in the Dockerfile, like in the average algorithm example.

At the moment, environment variables are used to pass to the algorithm container the details of the database to use. This means that the database label will become a environment variable name.

While bash seems to be OK loading env vars with a "-", dash is, ironically, not :)

$ docker run -e 'EXAMPLE-HERE=grepme' python:3.10-slim-bullseye /bin/bash -c 'env' | grep grepme
EXAMPLE-HERE=grepme
$ docker run -e 'EXAMPLE-HERE=grepme' python:3.10-slim-bullseye /bin/dash -c 'env' | grep grepme
$

Dash only allows valid identifiers: "a letter or underscore followed by zero or more letters, underscores, and digits".

[lsago]

For now I've pushed a small commit to our fork, but can just push in a branch to vantage6/vantage6 if that's preferable. Also using small issue to get acquainted with your development workflow. :)

[bartvanb]

Hi Luis, thanks for reporting and also solving this! I think it would be best if you could also create a pull request here. We usually ship non-high-prio bugs such as these in the next release so in that case it would be best to create a PR of your branch to branch release/4.1. Note that we are probably going to release 4.1 early next week, so if you need some more time we can also do it in 4.2 or a patch release

[lsago]

I was having some issues with setting up testing for client, and I think I'll only have time to take at look at again towards the end of the week. Since it's a super minor issue and I'd like to use it (mostly) as an opportunity to go through this process (testing, your guys' git workflow, etc), I propose we leave it for a patch release (or 4.2).

For a patch release, to what branch would I do my PR from my branch in our fork? Also, so, do you folks prefer we work on branches in our fork and PR from there, or do you prefer we create branches in vantage6/vantage6 and PR from there? (If we are allowed).

[bartvanb]

Hi Luis, a branch in this repository is preferred, since we can then check out the branch most easily. I think you are allowed to do this as member of the vantage6 github organization.

We usually merge patch releases directly in main

[bartvanb]

There was a small issue with this PR which is to be resolved with #986