Skip to content

Conversation

@jy4096
Copy link
Contributor

@jy4096 jy4096 commented Sep 8, 2022

python notes:
run poetry shell before starting the development
gPRC notes:

  1. First is the proto file: The proto file is identical to the one in numaflow-go https://github.com/numaproj/numaflow-go/blob/main/pkg/apis/proto/function/v1/udfunction.proto. I copied and pasted it.
    (Copy the https://github.com/numaproj/numaflow-go/blob/main/pkg/apis/proto folder and paste at the repo root, rename it to protos)
  2. Step two: To generate the python proto code: from code repo root, run the following command.
python -m grpc_tools.protoc -I./protos/function/v1/ --python_out=pynumaflow/function/ --grpc_python_out=pynumaflow/function/  protos/function/v1/udfunction.proto

move the generated file to the generated folder
NOTE: need to manually add from pynumaflow.function.generated import udfunction_pb2 as udfunction__pb2 in the generated udfunction_grpc_pb2 file.
from pynumaflow.function.generated is needed to import udfunction_pb2 as mentioned in https://groups.google.com/g/grpc-io/c/hpPnAGY0ksQ.
server.py also imports from pynumaflow.function.generated import udfunction_pb2 as udfunction__pb2
If we find a proper fix we can replace the current from.. approach...

  1. Need to manually add a udfunction_pb2.pyi file for the data types defined in the proto file.
  2. Next is to add new datatypes in _dtypes.go (not sure if this is the correct file to put new data types)
  3. Implement the server.py
  4. Add an udfunction example: forward message
  5. Add tests

I refactor the code so that the function is using gRPC and sink is still using http.
Also need help with the code style/structure/design pattern etc.. not confident in those things

Thank you!

ref:
python gRPC testing pkg https://grpc.github.io/grpc/python/grpc_testing.html
python gRPC unit test example.. https://blog.masuqat.net/2018/04/16/grpc-python-unittest/
async graceful shutdown example https://github.com/lidizheng/grpc/blob/master/examples/python/helloworld/async_greeter_server_with_graceful_shutdown.py

jyu6 added 6 commits September 6, 2022 16:12
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #4 (40a2d79) into main (5de24c4) will increase coverage by 3.88%.
The diff coverage is 85.83%.

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   85.43%   89.31%   +3.88%     
==========================================
  Files          12       13       +1     
  Lines         309      337      +28     
  Branches       36       37       +1     
==========================================
+ Hits          264      301      +37     
+ Misses         40       34       -6     
+ Partials        5        2       -3     
Impacted Files Coverage Δ
pynumaflow/sink/decoder.py 100.00% <ø> (ø)
pynumaflow/sink/encoder.py 90.00% <ø> (ø)
pynumaflow/function/server.py 68.51% <68.51%> (ø)
pynumaflow/_constants.py 100.00% <100.00%> (ø)
pynumaflow/function/__init__.py 100.00% <100.00%> (ø)
pynumaflow/function/_dtypes.py 97.95% <100.00%> (+4.73%) ⬆️
pynumaflow/sink/_dtypes.py 95.12% <100.00%> (ø)
pynumaflow/sink/handler.py 80.88% <100.00%> (ø)
pynumaflow/types.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jy4096
Copy link
Contributor Author

jy4096 commented Sep 8, 2022

Need help on the repo structure... @vigith

jyu6 added 17 commits September 8, 2022 11:00
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
@jy4096 jy4096 marked this pull request as ready for review September 13, 2022 00:30
jyu6 added 4 commits September 12, 2022 18:50
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
jyu6 added 5 commits September 14, 2022 14:05
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
self._cleanup_coroutines.append(server_graceful_shutdown())
await server.wait_for_termination()

def start(self) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jyu6 added 2 commits September 14, 2022 16:24
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
@jy4096 jy4096 requested review from ab93 and vigith and removed request for ab93, vigith and whynowy September 14, 2022 23:28
@vigith vigith self-requested a review September 14, 2022 23:29
Copy link
Member

@ab93 ab93 left a comment

Choose a reason for hiding this comment

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

Some minor comments.

jyu6 added 4 commits September 15, 2022 13:34
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
Signed-off-by: jyu6 <juanlu_yu@intuit.com>
@jy4096 jy4096 requested a review from ab93 September 15, 2022 21:12
Copy link
Member

@ab93 ab93 left a comment

Choose a reason for hiding this comment

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

Looks good.

@ab93 ab93 merged commit 496543c into numaproj:main Sep 15, 2022
@jy4096 jy4096 mentioned this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants