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

Run scenarios with CHC enabled #92

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

c12i
Copy link
Contributor

@c12i c12i commented Sep 11, 2024

This PR addreses #69

@c12i c12i changed the title Holochain chc performance testing Run scenarios with CHC enabled Sep 11, 2024
@c12i c12i force-pushed the holochain-chc-performance-testing branch from 8c901de to 9287afb Compare September 12, 2024 14:43
@c12i c12i changed the title Run scenarios with CHC enabled Run write/read scenario with CHC enabled Sep 16, 2024
@c12i c12i marked this pull request as ready for review September 16, 2024 12:47
@c12i c12i requested a review from ThetaSinner September 16, 2024 13:40
.github/workflows/test.yaml Outdated Show resolved Hide resolved
@@ -4,3 +4,4 @@ network:
- type: webrtc
signal_url: "ws://localhost:4423"
bootstrap_service: "http://localhost:4422"
# chc_url: "http://localhost:8181"
Copy link
Member

Choose a reason for hiding this comment

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

What are we going to do about needing different config for some scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the embed_conductor_config function might need to be refactored slightly, it should return a type rather than the yaml in the form of a string, so that values can be mutated by scenarios that need custom conductor config different from the one in the yaml files.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the scenario modifying the config. We want to be able to run scenarios with different config. In this case that means we want to run with CHC on and off. We don't then want to be modifying the scenario code to run in different environments etc.

What would be really nice is if you could provide a conductor configuration "overlay". So the embed_conductor_config would load a base configuration then at runtime you could configure which overlay to load. It might make sense to merge those as conductor configuration objects rather than yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I am late, got distracted with other work

I'm not sure about the scenario modifying the config. We want to be able to run scenarios with different config. In this case that means we want to run with CHC on and off. We don't then want to be modifying the scenario code to run in different environments etc.

I see what you mean, what would a good compromise be? perhaps having -chcvariants of the existing yaml configs, not the cleanest approach.

Or rather than reading from the conductor config files, we programmatically build the conductor configs from the scenario, with an env var i.e CHC_ENABLED determining whether a chc_url should be set.

I am open to discuss this further

Copy link
Member

@ThetaSinner ThetaSinner Sep 20, 2024

Choose a reason for hiding this comment

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

When I'm talking about overlays, I mean this kind concept -> https://carvel.dev/ytt/

Not necessarily that tool but something along those lines. So we could have a with-chc.yaml and a with-dpki.yaml that we can choose to load.

I think choosing to load that with an environment variable like CHC_ENABLED is good. So the current macro can load all the config files and embed them. Then we can choose at runtime which ones we merge with the base conductor config.

I know that YAML merging is a bit of a pain in general but if it's purely additive for now, then it shouldn't be so bad I hope.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, would the contents of the with-chc.yaml and with-dpki.yaml only contain the CHC and DPKI configurations?

Copy link
Member

Choose a reason for hiding this comment

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

Exactly that, they'd just bring in the piece that they configure. So we might do other things the same way, like different tunings of the network or the conductor itself.

We probably couldn't easily guarantee that a given combination of overlays makes sense but I'm okay with leaving that up to the user for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. The only question I have now is how to run scenarios with CHC enabled via trycp. I believe the holoports would need to have the reference implementation running?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to deploy somewhere that the HoloPorts can see yes. We have a couple of servers but nowhere ideal really. I think this might need to be a question we ask to techops

.github/workflows/test.yaml Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@c12i c12i requested a review from ThetaSinner September 17, 2024 13:02
@c12i c12i changed the title Run write/read scenario with CHC enabled Run scenarios with CHC enabled Sep 19, 2024
@abe-njama abe-njama linked an issue Sep 26, 2024 that may be closed by this pull request
4 tasks
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
bindings/trycp_runner/src/macros.rs Outdated Show resolved Hide resolved
.github/workflows/test.yaml Show resolved Hide resolved
@c12i c12i force-pushed the holochain-chc-performance-testing branch from 0c98a98 to 3fb149b Compare October 29, 2024 08:06
@c12i c12i force-pushed the holochain-chc-performance-testing branch from 3fb149b to 7e0f152 Compare October 29, 2024 08:07
@c12i c12i force-pushed the holochain-chc-performance-testing branch from 95e2067 to 2a25434 Compare October 29, 2024 08:37
@c12i c12i force-pushed the holochain-chc-performance-testing branch from bde3f9c to 9603d9a Compare October 31, 2024 05:35
@c12i c12i force-pushed the holochain-chc-performance-testing branch from 9603d9a to 8c28d97 Compare October 31, 2024 05:38
@c12i c12i self-assigned this Oct 31, 2024
@c12i c12i requested a review from ThetaSinner October 31, 2024 07:54
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.

Read/write with CHC enabled
2 participants