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

[job submission] Temporarily make pydantic imports conditional #19827

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

jiaodong
Copy link
Member

Diff Summary

Current job submission module run on ray default but user might not have pydantic installed. This diff aims to make it lazy import and removed serve import.

Related issue number

closes #19792

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Copy link
Contributor

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively just add pydantic to ray default?

python/ray/_private/job_manager/job_manager.py Outdated Show resolved Hide resolved
@jiaodong
Copy link
Member Author

jiaodong commented Oct 28, 2021

Alternatively just add pydantic to ray default?

i don't see downside of this. where's the precise place i need to add it -- requirements_default.txt?

@jiaodong
Copy link
Member Author

nvm pydantic is surprisingly large https://pypi.org/project/pydantic/#files for how simple the problem it appears to be solving ... so i don't think adding it to ray default is an option

@simon-mo
Copy link
Contributor

Pydantic is large because it uses cython to compile the whole codebase. You should be able to add it to setup.py, look for extra_requires.

I think you should either make it part of ray[default] or add a field set for ray[jobs].

@edoakes
Copy link
Contributor

edoakes commented Oct 28, 2021

@richardliaw @ericl thoughts on adding pydantic as a dependency to ray[default]? We would like to use this for schema validation in job submission.

@jiaodong
Copy link
Member Author

so we discussed this during standup and decided to go with lazy import for now but switch to jsonschema soon. We might need pydantic again if we want to migrate dashboard modules to fastapi sometime.

@edoakes edoakes changed the title make job imports more self contained [job submission] Temporarily make pydantic imports conditional Oct 28, 2021
@richardliaw
Copy link
Contributor

richardliaw commented Oct 29, 2021 via email

@edoakes
Copy link
Contributor

edoakes commented Oct 29, 2021

@richardliaw it's like 30MiB...

@edoakes
Copy link
Contributor

edoakes commented Oct 29, 2021

Ok, hopefully the last merge conflict here... @jiaodong can you ping me if/when tests are passing?

@pcmoritz
Copy link
Contributor

There is another linting error, @architkulkarni can you fix that? This should be merged ASAP, it is currently blocking the dashboard from working on master :)

@edoakes
Copy link
Contributor

edoakes commented Oct 29, 2021

Waiting for CI 😴

@edoakes
Copy link
Contributor

edoakes commented Oct 29, 2021

test_client unrealted

@edoakes edoakes merged commit bb0ebb7 into ray-project:master Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants