Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What should python2 be for faculty.clients.environment.Specification now that Python 2 is not supported? #198

Open
sbalian opened this issue Dec 24, 2021 · 7 comments

Comments

@sbalian
Copy link
Contributor

sbalian commented Dec 24, 2021

I am getting faculty.clients.base.BadRequest: (<Response [400]>, None, None) when trying to create a new environment. Here is the spec ...

from faculty.clients import environment

script = "echo hello"

spec = (
    environment.Specification(
        apt=environment.Apt(packages=[]),
        bash=[environment.Script(script=script)],
        python=environment.PythonSpecification(
            python2=None,
            python3=environment.PythonEnvironment(
                pip=environment.Pip(extra_index_urls=[], packages=[]),
                conda=environment.Conda(channels=[], packages=[]),
            ),
        ),
    ),
)

I think it may be due to the python2 value. I pulled the schema from what I get from client.get. In the tests for this library though, this is not set to None as far as I can see. I also tried just setting it to the same value as for Python 3 and still got the same error. Perhaps this is because the Platform no longer supports Python 2 and this library has not been updated? I cannot see the Python 2 options on the UI btw.

@imrehg
Copy link
Member

imrehg commented Dec 24, 2021

Looking inside the code and dumping some debug entries, it seems like that the spec above results internally in the library in this schema

_EnvironmentCreateUpdate(name='hello', description='testing issue', specification=(Specification(apt=Apt(packages=[]), bash=[Script(script='echo hello')], python=PythonSpecification(python2=None, python3=PythonEnvironment(pip=Pip(extra_index_urls=[], packages=[]), conda=Conda(channels=[], packages=[])))),))

(I added some description and name to be able to call the environment client's .create)
but then the code dumps that into a JSON to send to the relevant API _EnvironmentCreateUpdateSchema().dump(content), and that dump's result is

{'name': 'hello', 'description': 'testing issue', 'specification': {}}

that is a missing spec.

Just a hunch, but I wonder if this is because a parsing failure on those Python environments and thus being filled in as None for both?

@imrehg
Copy link
Member

imrehg commented Dec 24, 2021

@sbalian okay, issue is 2 fold

Part 1: your spec as defined is a tuple, because of that trailing , before the last bracket, and hence the above thing I mentioned, it is not parsed to an environment correctly from that.

Part 2: python2=None is not accepted, but in other form, such as python2=[] could create an environment (shows up fine in the UI, but couldn't parse the return value in this SDK (so the error is on this side)
I think I run into this while testing python2 removal before, and unfortunately don't remember what the outcome of the discussion at that time was, likely some workaround.

The workaround now is to send in a valid, empty Python environment for Python 2 and then both the API accepts it, and it can be parsed back by the SDK as well (until it's updated to reflect the changes). E.g. this works, combining Part 1 and Part 2:

# can reuse this as needed
empty_python_environment = environment.PythonEnvironment(
    pip=environment.Pip(extra_index_urls=[], packages=[]),
    conda=environment.Conda(channels=[], packages=[]),
)

spec = environment.Specification(
    apt=environment.Apt(packages=[]),
    bash=[environment.Script(script=script)],
    python=environment.PythonSpecification(
        python2=empty_python_environment,
        python3=environment.PythonEnvironment(
            pip=environment.Pip(extra_index_urls=[], packages=[]),
            conda=environment.Conda(channels=[], packages=[]),
        ),
    ),
)

@sbalian
Copy link
Contributor Author

sbalian commented Dec 24, 2021

@imrehg Thanks! Sorry - the tuple bit was a result of bad copy and pasting here. [] works! So does setting it to the same value as the one for Python 3. None does not work.

@sbalian sbalian closed this as completed Dec 24, 2021
@sbalian
Copy link
Contributor Author

sbalian commented Dec 24, 2021

And @imrehg that comma, black on save! careful!

@imrehg
Copy link
Member

imrehg commented Dec 24, 2021

@sbalian I would probably still keep the issue open, as it's complicated, and you did stumble upon a real issue with it, even if there's a workaround.

Interestingly, my code is also after black and that didn't do the variable = ( ... <newlines> ... ) trick. :) Careful indeed!

@sbalian
Copy link
Contributor Author

sbalian commented Dec 24, 2021

Thanks @imrehg. 1 I checked black again and it does the correct job. The original post was just a bad copy/paste job. 2 I opened the issue again, but changed the title.

@sbalian sbalian reopened this Dec 24, 2021
@sbalian sbalian changed the title Cannot create a new environment python2 value for faculty.clients.environment.Specification Dec 24, 2021
@sbalian sbalian changed the title python2 value for faculty.clients.environment.Specification What should python2 be for faculty.clients.environment.Specification now that Python 2 is not supported? Dec 24, 2021
@sbalian
Copy link
Contributor Author

sbalian commented Dec 24, 2021

Another finding @imrehg

Consider this for creating a new environment and then trying to list the environments.

import os
import uuid

import faculty
from faculty.clients import environment

PROJECT_ID = uuid.UUID(os.environ["FACULTY_PROJECT_ID"])

empty_python_spec = environment.PythonEnvironment(
    pip=environment.Pip(extra_index_urls=[], packages=[]),
    conda=environment.Conda(channels=[], packages=[]),
)

spec = environment.Specification(
    apt=environment.Apt(packages=[]),
    bash=[environment.Script(script="")],
    python=environment.PythonSpecification(
        python2=[], python3=empty_python_spec,
    ),
)


environment_client = faculty.client("environment")
environment_client.create(
    PROJECT_ID, "test", spec,
)

environment_client.list(PROJECT_ID)

I have used [] for python2. The environment is created fine. But when calling list() it throws the following:

marshmallow.exceptions.ValidationError: {3: {'specification': {'python': {'Python2': {'pip': ['Missing data for required field.'], 'conda': ['Missing data for required field.']}}}}}

Now changing the [] to empty_python_spec works! (i.e., no validation error when listing anymore). NB make sure you delete the environment from the UI before testing it with a different value for python2.

So in summary, we should use an empty spec rather than []. But this raises the important issue that one bad request to one environment (that is actually not rejected) breaks functionality for all other environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants