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

Groundwork for Streaming Progress Updates from View Service #3559

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

hdevalence
Copy link
Member

Sketch of one possibility for #3549 for discussion. (No code changes as yet, only proto changes).

grod220
grod220 previously approved these changes Jan 3, 2024
Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

Just two comments from my end

Comment on lines +138 to +150
message BuildProgress {
// An approximate progress of the build, from 0 to 1.
float progress = 1;
}
// Signals that the transaction is complete.
message Complete {
// The finished transaction.
penumbra.core.transaction.v1alpha1.Transaction transaction = 1;
}
oneof status {
BuildProgress build_progress = 1;
Complete complete = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these can be unnested so it can be shared with WitnessAndBuildResponse too

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, feel free to pick it up, we'll also have to propagate these changes through the rest of the source code.

// Signals that building is in progress.
message BuildProgress {
// An approximate progress of the build, from 0 to 1.
float progress = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this correlate to the % of actions that have been built thus far?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up to the implementation, I guess, in principle an implementation that had insight into how much proving was done as it was happening could give better updates. I believe there's a way to get Rayon to do this on the Rust side. I think at the proto level we should be as agnostic as possible, and initially, we should just try to expose info we have rather than adding progress plumbing into the proving internals.

@hdevalence
Copy link
Member Author

There's something off with the CI, this PR is green but it should definitely be red, it's half-finished and only changes protos, doesn't recompile them or propagate changes through the rest of the codebase. @conorsch any ideas?

@grod220 grod220 dismissed their stale review January 3, 2024 23:22

Working on updating implementations

@grod220 grod220 self-assigned this Jan 3, 2024
@conorsch
Copy link
Contributor

conorsch commented Jan 4, 2024

this PR is green but it should definitely be red, it's half-finished and only changes protos

Good catch, I think I broke this back in #3297, which altered the behavior so that the buf checks were run only when the PR is first opened, but not every time it's changed. Updated the logic in #3575, once that's in let's rebase this one and confirm we see the buf checks again!

@grod220 grod220 removed their assignment Jan 4, 2024
@zbuc zbuc force-pushed the tx-build-status branch from 8210c17 to 2c1ec95 Compare January 8, 2024 21:59
@aubrika aubrika linked an issue Jan 11, 2024 that may be closed by this pull request
@zbuc zbuc changed the title proto: (wip) progress updates from view service Groundwork for Streaming Progress Updates from View Service Jan 12, 2024
@aubrika
Copy link
Contributor

aubrika commented Jan 18, 2024

Blocked on #3621

WIP
Compiles
BroadcastTransactionResponse
Get code and tests compiling
lint
BroadcastTransaction
comment next steps
Rebuild proto
Remove unused imports

N.B. this commit was squashed by @conorsch
conorsch added a commit that referenced this pull request Jan 18, 2024
Tracking resolution in [0], disabling the test for now,
so we can merge [1] and unblock the web team.

[0] #3621,
[1] #3559
Tracking resolution in [0], disabling the test for now,
so we can merge [1] and unblock the web team.

[0] #3621,
[1] #3559
@conorsch
Copy link
Contributor

CI is now passing on this PR, module the protobuf lint failure, which is to be expected, because the proto changes here are Breaking Changes™. Will continue investigation of sweep test in #3621. Proceeding with merge here to unblock web impl consuming this new interface.

@conorsch conorsch merged commit 5b4fb16 into main Jan 18, 2024
6 of 7 checks passed
@conorsch conorsch deleted the tx-build-status branch January 18, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Change View RPCs to allow streaming progress indicators during build
5 participants