coiled / feedback

A place to provide Coiled feedback
14 stars 3 forks source link

undocumented restriction on software environment name: uppercase not allowed #94

Closed jgawad-nlmk closed 3 years ago

jgawad-nlmk commented 3 years ago

I am trying to create a software environment using Python API, and I encounter problems.

The code below terminates with an exception:

soft_env = coiled.create_software_environment('ML-test', conda={'channels': ['defaults', 'conda-forge'], 'dependencies': ['dask', 'pandas', 'scikit-learn', 'lightgbm', 'sklearn-contrib-py-earth']})

The exception is:

ValueError: Unable to update Environment: An error occurred (InvalidParameterException) when calling the CreateRepository operation: Invalid parameter at 'repositoryName' failed to satisfy constraint: 'must satisfy regular expression '(?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*''

It seems the problem is related to the uploading of the image. Below the printout of the last operations emitted by the call to create_software_environment:

STEP 4: SHELL ["conda", "run", "-n", "base", "/bin/bash", "-c"]
STEP 5: COMMIT 78e77eb9-b70c-40c1-82fb-8ff118638181
--> 0fc0e263a6d
0fc0e263a6de31f9b37b0a2c9e568fc2318ca25bac9308ddbd29268fae0b456b
Docker build succeeded: 78e77eb9-b70c-40c1-82fb-8ff118638181
Uploading image
An error occurred (InvalidParameterException) when calling the CreateRepository operation: Invalid parameter at 'repositoryName' failed to satisfy constraint: 'must satisfy regular expression '(?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*''
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\coiled\core.py", line 1402, in create_software_environment
    return cloud.create_software_environment(
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\coiled\core.py", line 544, in create_software_environment
    return self._sync(  # type: ignore
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\coiled\core.py", line 280, in _sync
    return sync(
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\distributed\utils.py", line 340, in sync
    raise exc.with_traceback(tb)
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\distributed\utils.py", line 324, in f
    result[0] = yield future
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\tornado\gen.py", line 762, in run
    value = future.result()
  File "C:\Users\j.gawad\Miniconda3\envs\coiled\lib\site-packages\coiled\core.py", line 657, in _create_software_environment
    raise ValueError(f"Unable to update Environment: {error_details}")
ValueError: Unable to update Environment: An error occurred (InvalidParameterException) when calling the CreateRepository operation: Invalid parameter at 'repositoryName' failed to satisfy constraint: 'must satisfy regular expression '(?:[a-z0-9]+(?:[._-][a-z0-9]+)*/)*[a-z0-9]+(?:[._-][a-z0-9]+)*''

It looks like a result of (too) restrictive validation on the environment name, which does not permit uppercase letters. The code below, which differs only in the name of the environment (ml-test in place of ML-test), succeeds:

>>> soft_env = coiled.create_software_environment(name='ml-test', conda={'channels': ['defaults', 'conda-forge'], 'dependencies': ['dask', 'pandas', 'scikit-learn', 'lightgbm', 'sklearn-contrib-py-earth']})

[long printout omitted for brevity]
Docker build succeeded: abb70aee-9b83-44d6-ae82-ffac1f67da45
Uploading image
Getting image source signatures
Copying blob sha256:fe122945008d986aaf0ae2bf10e192e5676222efc276c2d88e5785f28ba79246
Copying blob sha256:36f3d898946856d5344dff00e94aabd5cdac12b25515b5fb0ff7a436012846e5
Copying blob sha256:42851133c65c2a965b74a272b1737184e3a0b40c2b02ea29a6a9e9dc45d43971
Copying blob sha256:fcd8d39597dd39d0c68670479e4d240fa9dba04a1246587384df9e1aa31b17d4
Copying blob sha256:875120aa853cf59c6c5bc24af9f448a55f9b64db0bab58c9ee18f8a92ed8ac33
Copying blob sha256:f2cb0ecef392f2a630fa1205b874ab2e2aedf96de04d0b8838e4e728e28142da
Copying config sha256:0fc0e263a6de31f9b37b0a2c9e568fc2318ca25bac9308ddbd29268fae0b456b
Writing manifest to image destination
Storing signatures
Finished updating environment

I cannot find any restrictions on the software environment name mentioned in the documentation of software environments. Perhaps they should be explicitly stated in the docs? Or maybe they would be automatically enforced by converting software environment name to lowercase? I guess the restriction comes from naming rules of image registry. Still, an end-user should not be bothered by peculiarities of technologies that Coiled uses under the hood.

FabioRosado commented 3 years ago

Hello @jgawad-nlmk apologies for the late reply and thank you for raising this issue to us. I can understand that this issue that you encountered might have taken you a bit of time to figure out why that ValueError exception was raised.

I will tackle this issue and get a fix as soon as possible!

FabioRosado commented 3 years ago

Hello again, I just wanted to give you some more information about this issue. We have just merged a fix to both the documentation and the codebase.

The documentation now clearly mentions that the environment name must be lowercase (this was added to different places in the documentation).

We will also lowercase the name when creating a new environment - this is also documented just in case folks expect a different environment name (since uppercase letters where used).

Again thank you so much for reporting this issue to us and for figuring out what was happening with that error. This fix will be available on the next release, so I'm going to close this issue now, but feel free to comment here or open a new one if you encounter any issues.

jgawad-nlmk commented 3 years ago

Hello @FabioRosado, many thanks for fixing this!