Skip to content

[Serve] Fix return type of _DeploymentHandleBase._get_or_create_router#46480

Merged
edoakes merged 1 commit intoray-project:masterfrom
JoshKarpel:fix-_get_or_create_router-type
Jul 11, 2024
Merged

[Serve] Fix return type of _DeploymentHandleBase._get_or_create_router#46480
edoakes merged 1 commit intoray-project:masterfrom
JoshKarpel:fix-_get_or_create_router-type

Conversation

@JoshKarpel
Copy link
Contributor

@JoshKarpel JoshKarpel commented Jul 8, 2024

Why are these changes needed?

I'm definitely not using this private method in a hack in our code, and mypy is complaining about it.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

Signed-off-by: Josh Karpel <josh.karpel@gmail.com>
)

def _get_or_create_router(self) -> Union[Router, asyncio.AbstractEventLoop]:
def _get_or_create_router(self) -> Tuple[Router, asyncio.AbstractEventLoop]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See below, this returns a tuple of these two types, not one-or-the-other

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, probably just a typo :)

really wish we were actually using mypy on the serve codebase...

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 would love that!

It's definitely tricky around the actual .remote() calls and whatnot that users would see, but it would be great just for internal development.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah most of the code tries to abstract out the ray remote calls, so it shouldn't be hard in theory

@JoshKarpel JoshKarpel marked this pull request as ready for review July 8, 2024 18:16
@JoshKarpel JoshKarpel changed the title Fix return type of _DeploymentHandleBase._get_or_create_router [Serve] Fix return type of _DeploymentHandleBase._get_or_create_router Jul 8, 2024
@akshay-anyscale akshay-anyscale requested review from a team and edoakes July 11, 2024 16:09
Copy link
Member

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

lol!

@edoakes edoakes enabled auto-merge (squash) July 11, 2024 18:15
@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label Jul 11, 2024
@edoakes edoakes merged commit 306cd9e into ray-project:master Jul 11, 2024
@JoshKarpel JoshKarpel deleted the fix-_get_or_create_router-type branch July 11, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants