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: Run scenarios with CHC enabled #92

Draft
wants to merge 35 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
Copy link

cocogitto-bot bot commented Feb 6, 2025

❌ Found 0 compliant commit and 34 non-compliant commits in 9d851bf...a5581c3.

Commit 9d851bf by @c12i is not conform to the conventional commit specification :

  • message: add chc-service nix package
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | add chc-service nix package
          |    ^---
          |
          = expected scope or type_separator
    

Commit 4ab6425 by @c12i is not conform to the conventional commit specification :

  • message: nix flake update
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | nix flake update
          |    ^---
          |
          = expected scope or type_separator
    

Commit 8a58cbb by @c12i is not conform to the conventional commit specification :

  • message: update holonix
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | update holonix
          |       ^---
          |
          = expected scope or type_separator
    

Commit 230fa4f by @c12i is not conform to the conventional commit specification :

  • message: initial scenario setup
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:8
          |
        1 | initial scenario setup
          |        ^---
          |
          = expected scope or type_separator
    

Commit 5a28705 by @c12i is not conform to the conventional commit specification :

  • message: nix flake update
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | nix flake update
          |    ^---
          |
          = expected scope or type_separator
    

Commit 8505e06 by @c12i is not conform to the conventional commit specification :

  • message: nix flake update
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | nix flake update
          |    ^---
          |
          = expected scope or type_separator
    

Commit 3a51e5f by @c12i is not conform to the conventional commit specification :

  • message: update conductor config
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | update conductor config
          |       ^---
          |
          = expected scope or type_separator
    

Commit 5438ca5 by @c12i is not conform to the conventional commit specification :

  • message: update ci workflow
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | update ci workflow
          |       ^---
          |
          = expected scope or type_separator
    

Commit a704319 by @c12i is not conform to the conventional commit specification :

  • message: update ci nixpkgs
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | update ci nixpkgs
          |       ^---
          |
          = expected scope or type_separator
    

Commit 32f4077 by @c12i is not conform to the conventional commit specification :

  • message: add script to run sandbox conductor and chc service concurrently
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | add script to run sandbox conductor and chc service concurrently
          |    ^---
          |
          = expected scope or type_separator
    

Commit 9287afb by @c12i is not conform to the conventional commit specification :

  • message: run run.sh as a background process in ci
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | run run.sh as a background process in ci
          |    ^---
          |
          = expected scope or type_separator
    

Commit ecbb71b by @c12i is not conform to the conventional commit specification :

  • message: fix CI; only start chc service on write_read scenario
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | fix CI; only start chc service on write_read scenario
          |    ^---
          |
          = expected scope or type_separator
    

Commit 7665694 by @c12i is not conform to the conventional commit specification :

  • message: wait for CHC service to be available before running sandbox
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:5
          |
        1 | wait for CHC service to be available before running sandbox
          |     ^---
          |
          = expected scope or type_separator
    

Commit 0e098b4 by @c12i is not conform to the conventional commit specification :

  • message: split write_read scenarios
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | split write_read scenarios
          |      ^---
          |
          = expected scope or type_separator
    

Commit d4e1cd8 by @c12i is not conform to the conventional commit specification :

  • message: revert inadvertedly removed newlines at the end of file
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | revert inadvertedly removed newlines at the end of file
          |       ^---
          |
          = expected scope or type_separator
    

Commit 6301aec by @c12i is not conform to the conventional commit specification :

  • message: remove commented out chc_url from conductor config yaml files
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | remove commented out chc_url from conductor config yaml files
          |       ^---
          |
          = expected scope or type_separator
    

Commit b550110 by @c12i is not conform to the conventional commit specification :

  • message: remove overridden holochain flake input
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | remove overridden holochain flake input
          |       ^---
          |
          = expected scope or type_separator
    

Commit 96b0635 by @c12i is not conform to the conventional commit specification :

  • message: revert newlines in conductor config yaml file
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | revert newlines in conductor config yaml file
          |       ^---
          |
          = expected scope or type_separator
    

Commit e145c02 by @c12i is not conform to the conventional commit specification :

  • message: run more scenarios with CHC enabled
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | run more scenarios with CHC enabled
          |    ^---
          |
          = expected scope or type_separator
    

Commit 218d62b by @c12i is not conform to the conventional commit specification :

  • message: Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
          |      ^---
          |
          = expected scope or type_separator
    

Commit 21300d9 by @c12i is not conform to the conventional commit specification :

  • message: derive a with-chc conductor config and embed conditionally to base configs
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | derive a with-chc conductor config and embed conditionally to base configs
          |       ^---
          |
          = expected scope or type_separator
    

Commit bc11dfb by @c12i is not conform to the conventional commit specification :

  • message: Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
          |      ^---
          |
          = expected scope or type_separator
    

Commit adedffb by @c12i is not conform to the conventional commit specification :

  • message: update workspace.nix
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | update workspace.nix
          |       ^---
          |
          = expected scope or type_separator
    

Commit 7e0f152 by @c12i is not conform to the conventional commit specification :

  • message: Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
          |      ^---
          |
          = expected scope or type_separator
    

Commit 6f84ccb by @c12i is not conform to the conventional commit specification :

  • message: Remove some CHC test runs
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | Remove some CHC test runs
          |       ^---
          |
          = expected scope or type_separator
    

Commit 2a25434 by @c12i is not conform to the conventional commit specification :

  • message: Clean up embed_conductor_config
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Clean up embed_conductor_config
          |      ^---
          |
          = expected scope or type_separator
    

Commit c720e1b by @c12i is not conform to the conventional commit specification :

  • message: Fix fmt
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Fix fmt
          |    ^---
          |
          = expected scope or type_separator
    

Commit b243b05 by @c12i is not conform to the conventional commit specification :

  • message: Fix nonCargoBuildFiles
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Fix nonCargoBuildFiles
          |    ^---
          |
          = expected scope or type_separator
    

Commit 8c28d97 by @c12i is not conform to the conventional commit specification :

  • message: Update conductor-config
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | Update conductor-config
          |       ^---
          |
          = expected scope or type_separator
    

Commit a53b456 by @c12i is not conform to the conventional commit specification :

  • message: Enable the chc feature on holochain
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:7
          |
        1 | Enable the chc feature on holochain
          |       ^---
          |
          = expected scope or type_separator
    

Commit 1ca424f by @c12i is not conform to the conventional commit specification :

  • message: Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Merge remote-tracking branch 'holochain/main' into holochain-chc-performance-testing
          |      ^---
          |
          = expected scope or type_separator
    

Commit ade10a4 by @c12i is not conform to the conventional commit specification :

  • message: Add instruction for running the CHC service in the README
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Add instruction for running the CHC service in the README
          |    ^---
          |
          = expected scope or type_separator
    

Commit 9129fff by @c12i is not conform to the conventional commit specification :

  • message: Merge branch 'main' into holochain-chc-performance-testing
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:6
          |
        1 | Merge branch 'main' into holochain-chc-performance-testing
          |      ^---
          |
          = expected scope or type_separator
    

Commit a5581c3 by @c12i is not conform to the conventional commit specification :

  • message: Nix flake update
  • cause:
    Missing commit type separator `:`
    
    Caused by:
         --> 1:4
          |
        1 | Nix flake update
          |    ^---
          |
          = expected scope or type_separator
    

@c12i c12i changed the title Run scenarios with CHC enabled feat: Run scenarios with CHC enabled Feb 6, 2025
@c12i c12i marked this pull request as draft February 6, 2025 09:16
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