-
Notifications
You must be signed in to change notification settings - Fork 206
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(orchestration): stopgap integration w asyncFlow #9521
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with Cloudflare Pages
|
turadg
force-pushed
the
9281-provideAsyncTools
branch
from
June 18, 2024 01:11
05cd7ee
to
f096604
Compare
erights
force-pushed
the
markm-stopgap-orch-use-async-flow
branch
from
June 18, 2024 15:45
6db8022
to
68908e3
Compare
mhofman
reviewed
Jun 18, 2024
Comment on lines
+243
to
+245
const guestFunc = async (...args) => fn(orc, ctx, ...args); | ||
const hostFunc = asyncFlow(zone, durableName, guestFunc); | ||
return hostFunc; |
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.
Here is what I had in my draft:
Suggested change
const guestFunc = async (...args) => fn(orc, ctx, ...args); | |
const hostFunc = asyncFlow(zone, durableName, guestFunc); | |
return hostFunc; | |
const subzone = zone.subZone(durableName); | |
// TODO: create durable exo wrappers for ctx props | |
const ctxKit = {}; // makeDurableCtxKit(ctx); | |
const asyncFlowGuestFunc = async (flowSupportKit, ...args) => { | |
// TODO: Add support for recreating a `this` exo class context | |
const exoContext = undefined; | |
// TODO: Get orc from `flowSupportKit` once it's made durable | |
const orcArg = orc; | |
// TODO: build ctx from a durable skeleton using `flowSupportKit` | |
const ctxArg = ctx; | |
return apply(fn, exoContext, [orcArg, ctxArg, ...args]); | |
} | |
const asyncFlowHostFn = asyncFlow(subzone, 'asyncFlow', asyncFlowGuestFunc); | |
return harden({ | |
[durableName](...args) { | |
// TODO: Build the needed durable objects | |
const orcKit = {}; // internalMakeOrchestratorKit(); | |
// TODO: plumb the necessary ingredients to reconstruct a `this` exo context inside the guest | |
return asyncFlowHostFn({ orc: orcKit.orchestrator, ctxKit }, ...args); | |
}, | |
}[durableName]); |
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.
See #9566 , where this work continues.
mergify bot
added a commit
that referenced
this pull request
Jun 19, 2024
refs: #9281 ## Description AsyncFlow requires that everything passing the membrane is durable. This makes the facade objects durable to conform. Doing so for `localChainFacade` is deferred so we can get this into master sooner, to aid @erights 's #9521 . ### Security Considerations none ### Scaling Considerations Exo for each chain and each account ### Documentation Considerations none ### Testing Considerations Existing coverage ### Upgrade Considerations none, not yet deployed
erights
added
the
asyncFlow
related to membrane-based replay and upgrade of async functions
label
Jun 20, 2024
Closing in favor of #9566 |
mergify bot
pushed a commit
that referenced
this pull request
Jun 25, 2024
closes: #XXXX refs: #9449 #9521 #9304 #9281 ## Description Changed async-flow to support endowments. Changed `orchestrate` to use `asyncFlow` with endowments. Changed `sendAnywhere` example orchestration contract to be more compatible with this new `orchestrate`. The CI errors are all in the `orchestration` package. After some earlier iteration where orchestration failures indicated async-flow bugs, which I fixed, the remaining errors seem plausibly to be integration bugs on the orchestration side revealed by using this improved `orchestrate` function. If so, that satisfies the purpose of this PR -- to enable integration testing to reveal such errors. However, this leaves open the question of how to bring this PR to fruition despite these CI errors. In that iteration, the majority of errors were due to host-side promises, which we expected. To proceed with integration testing, I temporarily turned that case into a warning, by wrapping the host-side promise with a host-side vow. This stopgap measure is obviously fragile under upgrade. It would cause may upgrades to fail However, I have not investigated these CI errors enough to be at all confident that none of them are due to bugs in async-flow. For any of those, they should be fixed in this PR. ### Security Considerations nothing new ### Scaling Considerations none, given that total endowments are low cardinality. All these endowments are prepare-time per-function. There should not be any cardinality limit on the activations making use of these endowments. But like all other async-flow scaling issues, that remains to be tested. ### Documentation Considerations The endowment rules and taxonomy is interesting, and should be documented. ### Testing Considerations We get CI errors only from the `orchestration` package. Some of these may be the integration bugs we wanted this exercise to reveal. However, others may be async-flow bugs, which should have been caught by async-flow unit tests. The warning stopgap I mentioned above [appears in CI](https://github.com/Agoric/agoric-sdk/actions/runs/9637015639/job/26575694851?pr=9566#step:12:648) as, for example ``` Warning for now: vow expected, not promise Promise { <pending> } (Error#1) Error#1: where warning happened at makeGuestForHostVow (.../async-flow/src/replay-membrane.js:329:9) at eval (.../async-flow/src/convert.js:119:10) at innerConvert (.../async-flow/src/convert.js:63:8) at convertRecur (.../async-flow/src/convert.js:30:8) at convert (.../async-flow/src/convert.js:76:1) at performCall (.../async-flow/src/replay-membrane.js:137:1) at guestCallsHost (.../async-flow/src/replay-membrane.js:195:9) at In "getChain" method of (Orchestrator orchestrator) [as getChain] (.../async-flow/src/replay-membrane.js:282:8) at eval (.../orchestration/src/examples/unbondExample.contract.js:60:23) at eval (.../async-flow/src/async-flow.js:222:1) at Object.restart (.../async-flow/src/async-flow.js:222:30) at makeAsyncFlowKit (.../async-flow/src/async-flow.js:430:6) at asyncFlow_hostFlow (.../async-flow/src/async-flow.js:448:13) at orcFn (.../orchestration/src/facade.js:124:15) at eval (.../pass-style/src/make-far.js:224:31) ``` The relevant lines are ``` at In "getChain" method of (Orchestrator orchestrator) [as getChain] (.../async-flow/src/replay-membrane.js:282:8) at eval (.../orchestration/src/examples/unbondExample.contract.js:60:23) ``` where the first line indicates what method or method guard provided the inappropriate promise ```js getChain: M.callWhen(M.string()).returns(ChainInfoShape), ``` and the second line indicates where the guest code called it ```js const omni = await orch.getChain('omniflixhub'); ``` ### Upgrade Considerations The orchestration code in question cannot be truly upgrade safe until we see no more of these "vow expected, not promise" warnings. Even then, we should expect that async-flow as of this PR is ready for lots of testing, but not yet ready to run on the main chain with durable state expected to survive real upgrades.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #XXXX
refs: #XXXX
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations