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.

Sign up for GitHub

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

Jump to bottom

[Serve] add type hints for controller and backend_worker #10288

Merged
merged 24 commits into from
Aug 27, 2020
Merged

[Serve] add type hints for controller and backend_worker #10288

merged 24 commits into from
Aug 27, 2020

Conversation

architkulkarni
Copy link
Contributor

Why are these changes needed?

Adds type hints for controller.py and backend_worker.py

Related issue number

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/latest/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested (please justify below)

simon-mo
simon-mo approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@simon-mo simon-mo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with two minor things

python/ray/serve/backend_worker.py Outdated
backend_config: BackendConfig,
instance_name=None):
instance_name: str = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional[str]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed

python/ray/serve/backend_worker.py
@@ -140,7 +141,7 @@ def wrap_to_ray_error(exception):
return ray.exceptions.RayTaskError(str(e), traceback_str, e.__class__)


def ensure_async(func):
def ensure_async(func: Callable) -> Callable:
Copy link
Contributor

Choose a reason for hiding this comment

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

-> AsyncCallable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't find AsyncCallable in the project or on google, I made it Coroutine instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. Sorry I confused types in typing and those in inspect. Coroutine is not actually correct. Coroutine are things you get after call call an async func and it can be awaited on. We are typing async in this case. Can we just revert the change, back to Callable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, done

@simon-mo simon-mo added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 24, 2020
@architkulkarni architkulkarni removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 24, 2020
@architkulkarni architkulkarni added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 24, 2020
@architkulkarni
Copy link
Contributor Author

The CI is failing because of some bug with pickle not being able to pickle Union[str, None]. According to facebookresearch/hydra#428 this is fixed in a new version of cloudpickle. What should we do here?

@simon-mo
Copy link
Contributor

simon-mo commented Aug 25, 2020 •
edited
Loading

Thanks for tracking this down. Let's drop the type hints in RayServeWrappedWorker. That should make the test pass. Upgrading cloudpickle is much more involved, we can definitely add types back to RayServeWrappedWorker after upgrading cloudpickle.

@architkulkarni architkulkarni removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2020
@architkulkarni architkulkarni added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2020
simon-mo
simon-mo reviewed Aug 25, 2020
View reviewed changes
python/ray/serve/api.py Outdated
@@ -56,7 +56,8 @@ def __call__(self, *, python_arg=None):
return f


def init(name: Optional[str] = None,
# TODO(architkulkarni): Add type hint for name after upgrading cloudpickle.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine. I think cloudpickle only breaks for Optional in generated class like RayServeWrappedWorker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, that makes sense. I forgot that the tests for the type hints for API were passing earlier. I'll revert this change

simon-mo
simon-mo reviewed Aug 25, 2020
View reviewed changes
python/ray/serve/controller.py Outdated
Comment on lines 651 to 653
async def create_endpoint(
self, endpoint: str, traffic_dict: Dict[str, float],
route: Optional[str], methods: List[str]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where the build error come from.

@architkulkarni architkulkarni added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Aug 25, 2020
@simon-mo simon-mo merged commit eea7a86 into ray-project:master Aug 27, 2020
@architkulkarni architkulkarni deleted the serve-type-hints-all branch August 27, 2020 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon-mo simon-mo simon-mo approved these changes

Assignees

@simon-mo simon-mo

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@architkulkarni @simon-mo