-
Notifications
You must be signed in to change notification settings - Fork 1
Initial implementation #1
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
base: main
Are you sure you want to change the base?
Conversation
a0db8c4
to
2733044
Compare
4c284df
to
9bdf24d
Compare
fbb91b0
to
659557b
Compare
96f9be3
to
9711999
Compare
fa76eac
to
706b354
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started adding some more comments here but started seeing a lot of past comments that still apply so unsure if ready for second round. Stopped at about src/nexus_rpc/nexusrpc/handler/_core.py
but will pick back up when told ready for re-review.
.github/workflows/ci.yml
Outdated
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ['3.9', '3.10', '3.11', '3.12', '3.13', '3.14'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump here
src/nexusrpc/_serializer.py
Outdated
User provided keys are treated case-insensitively. | ||
""" | ||
|
||
data: Optional[bytes] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedantic, but I don't think we should default to None
, I think any instantiator of this should explicitly pass None
if they have no data, but creating a Content
should always required data
IMO. Same for not having default stream
on LazyValue
constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[
Content
] ... Pedantic, but I don't think we should default toNone
Agree, done.
src/nexusrpc/types.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there aren't enough types here to deserve its own user-facing module. Just put InputT
and OutputT
in the nexusrpc
module. The other ones don't really have any bounds/constraints, so there's not really any value in even having them be user facing, they can just be TypeVar
s where needed as needed (or shared across files, but don't need to be user facing).
@@ -0,0 +1,133 @@ | |||
# See | |||
# https://docs.python.org/3/howto/annotations.html#accessing-the-annotations-dict-of-an-object-in-python-3-9-and-older |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.9 will be EOL by October. I'd be ok if we went ahead and said Python Nexus is 3.10+ and remove any 3.9 polyfills/workarounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK let's consider this for Preview; added to list
provided, the default behavior for the error type is used. | ||
""" | ||
super().__init__(message) | ||
self.__cause__ = cause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrmm, Python exceptions do not usually accept cause in constructors, they usually rely on the from
clause during raise
or in the rare case of needing to create the exception with a cause without a raise
, users manually set __cause__
themselves after creation. Having it in constructor is confusing, especially in cases where it's unclear it may override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I haven't resolved this yet.
|
||
def __init__( | ||
self, | ||
user_service_handlers: Sequence[Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a base handler, why accept these? What if I want to implement my base handler without any user service handlers? I think we should have a true abstract handler with no implementation defaults (i.e. basically an interface). Can we move all of this constructor stuff to the Handler
class and get rid of this constructor? If we need some kind of shared utilities between Handler
and SyncHandler
so they can accept similar constructor arguments, that can make sense (or even a shared base class), but IMO we still need a no-impl/no-constructor interface for handlers.
# TODO(prerelease): we have a syncio module now housing the syncio version of | ||
# SyncOperationHandler. If we're retaining that then this (and an async version of | ||
# LazyValue) should go in there. | ||
class SyncioHandler(BaseHandler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a use case for this at this time? If so, we should eagerly check at construction that all calls on all operation handlers are not async def
based. And we should require the executor on constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a use case for this at this time?
How else will a team using a non-async/await framework such as Django implement a Nexus server/worker?
if not self.executor: | ||
raise RuntimeError( | ||
"Operation start handler method is not an `async def` but " | ||
"no executor was provided to the Handler constructor. " | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we detect this at construction time? Meaning can we check in the constructor that if an executor
is not present, all calls on all operation handlers are async def
?
This reverts commit 95f7874.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
||
on: | ||
push: | ||
branches: [ main, v0 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why v0
? Is it to run CI in this PR? Will you remove it eventually?
|
||
jobs: | ||
test: | ||
# TODO(preview): other platforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan on addressing this or tracking in an issue?
- name: Build API docs | ||
run: uv run pydoctor src/nexusrpc | ||
|
||
- name: Deploy prod API docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this set up? What domain are you planning to push to? I was planning on using GH docs for TS for now.
run: uv run pydoctor src/nexusrpc | ||
|
||
- name: Deploy prod API docs | ||
if: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should publish docs on a stable release not on push to main. We made this mistake with Temporal SDKs, let's not repeat.
uv run pyright | ||
uv run mypy --check-untyped-defs . | ||
uv run ruff check --select I | ||
uv run ruff format --check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can all of this be packaged in one command?
|
||
from typing import TypeVar | ||
|
||
# Operation input type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure these comments are considered docstrings?
BAD_REQUEST = "BAD_REQUEST" | ||
UNAUTHENTICATED = "UNAUTHENTICATED" | ||
UNAUTHORIZED = "UNAUTHORIZED" | ||
NOT_FOUND = "NOT_FOUND" | ||
RESOURCE_EXHAUSTED = "RESOURCE_EXHAUSTED" | ||
INTERNAL = "INTERNAL" | ||
NOT_IMPLEMENTED = "NOT_IMPLEMENTED" | ||
UNAVAILABLE = "UNAVAILABLE" | ||
UPSTREAM_TIMEOUT = "UPSTREAM_TIMEOUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth documenting each of these error types.
) | ||
if self.operation_handlers: | ||
msg += f": {', '.join(sorted(self.operation_handlers.keys()))}" | ||
msg += "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it common to put periods at the end of error messages in Python? Doesn't seem like the standard library does this.
# TODO(prerelease): Do we want to require users to create this wrapper? Two | ||
# alternatives: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just use the standard concurrent futures executor interface here.
) | ||
|
||
method_name = get_callable_name(start) | ||
operation_handler_factory.__nexus_operation__ = nexusrpc.Operation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the factory need to have this attribute?
Nexus Python SDK initial content.
For an overview of the user experience of implementing and calling Nexus handlers, see the Temporal sample, and tests in this PR.