facultyai / faculty

A Python library for interacting with the Faculty platform
https://faculty.ai/platform/
18 stars 5 forks source link

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

Open sbalian opened 2 years ago

sbalian commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

imrehg commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.