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

feat: streaming updates python client [MD-246] #8778

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

rb-determined-ai
Copy link
Contributor

This includes a new code generator.

Our existing code generation is proto -> openapi -> tweaks -> python, with an alternate proto -> go path for the server code.

This is more direct, it's go -> python; the go code is the source of truth and we can write the generator in go to use go's ast package.

Test Plan

Feature is not yet user-facing. Automated testing is sufficient.

Commentary

I configured the PR against the streaming updates "core functionality" feature branch lands for reviewability, but I plan on landing it to main after that branch lands.

@rb-determined-ai rb-determined-ai requested review from a team as code owners January 31, 2024 16:28
@rb-determined-ai rb-determined-ai requested review from tayritenour and maxrussell and removed request for a team January 31, 2024 16:28
@cla-bot cla-bot bot added the cla-signed label Jan 31, 2024
@rb-determined-ai rb-determined-ai requested review from corban-beaird and removed request for maxrussell January 31, 2024 17:58
@corban-beaird corban-beaird force-pushed the corban-beaird-streaming-updates-core-func branch from 80dc1ee to 1c35290 Compare February 8, 2024 22:59
Base automatically changed from corban-beaird-streaming-updates-core-func to main February 9, 2024 20:54
@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch from a0b958c to 87bd990 Compare February 9, 2024 21:08
Copy link

netlify bot commented Feb 9, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 045e508
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65e2551617b9010008213607

@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch from 87bd990 to e3c6e26 Compare February 14, 2024 00:21
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: Patch coverage is 51.87032% with 386 lines in your changes are missing coverage. Please review.

Project coverage is 47.21%. Comparing base (b99ad9f) to head (045e508).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8778      +/-   ##
==========================================
+ Coverage   47.19%   47.21%   +0.02%     
==========================================
  Files        1155     1162       +7     
  Lines      175115   175914     +799     
  Branches     2237     2237              
==========================================
+ Hits        82648    83064     +416     
- Misses      92309    92692     +383     
  Partials      158      158              
Flag Coverage Δ
backend 42.08% <0.00%> (-0.29%) ⬇️
harness 63.93% <80.46%> (+0.21%) ⬆️
web 42.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
harness/determined/common/streams/__init__.py 100.00% <100.00%> (ø)
master/internal/stream/projects.go 75.45% <ø> (ø)
harness/determined/core/_train.py 39.82% <0.00%> (ø)
harness/determined/common/streams/_util.py 60.00% <60.00%> (ø)
harness/determined/common/streams/wire.py 86.79% <86.79%> (ø)
harness/tests/common/streams/test_client.py 89.03% <89.03%> (ø)
harness/determined/common/detlomond.py 37.14% <37.14%> (ø)
harness/determined/common/streams/_client.py 81.39% <81.39%> (ø)
master/cmd/stream-gen/main.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

Copy link
Contributor

@gt2345 gt2345 left a comment

Choose a reason for hiding this comment

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

Works as expected

@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch from e3c6e26 to 6e82c5e Compare March 1, 2024 18:25
@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch 3 times, most recently from cde2786 to 3d45715 Compare March 1, 2024 20:12
overwritten if it would be modified.

If --stamp is provided, the STAMP file will be touched after a successful run, which is useful for
integration with build systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious how the stamp file is used for integration with build systems. Is there an example in the code base that I can take a look at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you know, looking at this... there isn't a good reason for a --stamp output with how this function behaves. I'll just remove this.

@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch from 3d45715 to 5af6e2b Compare March 1, 2024 21:53
@rb-determined-ai
Copy link
Contributor Author

I need to figure out the CI situation here before landing this.

The code generation may complicate the relationships between stages. Ugh.

This includes a new code generator.

Our existing code generation is proto -> openapi -> tweaks -> python,
with an alternate proto -> go path for the server code.

This is more direct, it's go -> python; the go code is the source of
truth and we can write the generator in go to use go's ast package.
@rb-determined-ai
Copy link
Contributor Author

I need to figure out the CI situation here before landing this.

The code generation may complicate the relationships between stages. Ugh.

no... that's not right.

CI is fine. Since we still commit generated code, the make -C master check step will additional check that the stream-gen --python output is correct. Nobody else has a reason to regenerate that file.

I will include make -C master stream-gen as part of the devcluster p keybinding though.

@rb-determined-ai rb-determined-ai force-pushed the rb/streaming-updates-client branch from 5af6e2b to 045e508 Compare March 1, 2024 22:22
@rb-determined-ai rb-determined-ai changed the title feat: streaming updates python client feat: streaming updates python client [MD-246] Mar 1, 2024
@rb-determined-ai rb-determined-ai merged commit 592a566 into main Mar 1, 2024
71 of 84 checks passed
@rb-determined-ai rb-determined-ai deleted the rb/streaming-updates-client branch March 1, 2024 23:04
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
This includes a new code generator.

Our existing code generation is proto -> openapi -> tweaks -> python,
with an alternate proto -> go path for the server code.

This is more direct, it's go -> python; the go code is the source of
truth and we can write the generator in go to use go's ast package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants