NationTech/harmony
5
0
Fork 2
Code Issues 37 Pull Requests 16 Actions Packages Projects Releases 1 Wiki Activity

refactor: Interpret score with a provided method on Score #100

Merged
letian merged 3 commits from interpret-score into master 2025-08-09 22:56:26 +00:00
Conversation 1 Commits 3 Files Changed 23 +118 -78
letian commented 2025-08-07 15:46:14 +00:00
Owner
Copy Link

This PR implements the option (2) described below with a bit of "hardcoded" instrumentation. The refactoring to better manage state will come in a future PR because of more decoupling needed (Installable, AlertReceiver, AlertSender, all make use of the InterpretStatus and such).

Context

In the current design, we execute the interpretation of a score as such:

let score = MyScore {};
let result = score.create_interpret().execute(inventory, topology);

The signature being:

trait Score<T: Topology> {
  fn create_interpret(&self) -> Box<dyn Interpret<T>>;
}

trait Interpret<T: Topology> {
  fn execute(&self, inventory: &Inventory, topology: &T) -> Result<InterpretOutcome, InterpretError>;
}

We can do it whenever we want, wherever we want.

Because of this, it is no easy task to control the lifecycle (status) of an Intrepret, without pushing this responsibility back to the developers of scores/interprets. Even if we try to have a "smart" interpret state that handle itself the lifecycle/instrumentation.

So after some consideration, here are the 2 options I explored that allows to keep the flexibility of executing interprets whenever, wherever we want as well as composing scores together.

Possible implementations

1. Introduce an interpret_score function

Plays a similar role to Maestro::prepare_topology by "orchestrating" the execution of an Interpret:

fn interpret_score(score: Score, topology: &T) -> Result<InterpretState, InterpretError> {
  let interpret = score.create_interpret();
  let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running }

  let result = interpret.execute(topology);

  match result => {
    Ok(outcome) => match outcome {
      InterpretOutcome::Success { details } => satus.success(details)
      InterpretOutcome::Noop { details } => satus.noop(details)
    }
    Err(err) => status.error(err.message)
  }

  result
}

// example
let score = MyScore {};
let result = interpret_score(score, topology);

Pros:

  • No need to change existing Score/Interpret implementations

Cons:

  • Adds an extra function
  • Doesn't force developers to use it

2. Add interpret directly to Score (as a provided method)

Similar to proposal (1), the difference is that it's part of the Score trait directly, making it easier to discover and use without needing to remember a separate function:

trait Score {
  fn interpret(&self, topology: &T) -> Result<InterpretState, InterpretError> {
    let interpret = self.create_interpret();
    let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running }

    let result = interpret.execute(topology);

    match result => {
      Ok(outcome) => match outcome {
        InterpretOutcome::Success { details } => satus.success(details)
        InterpretOutcome::Noop { details } => satus.noop(details)
      }
      Err(err) => status.error(err.message)
    }

    result
  }

  fn create_interpret(&self) -> impl Interpret;
}

// example
let score = MyScore {};
let result = score.interpret(topology);

Pros:

  • No need to change existing Score/Interpret implementations
  • Easier and more intuitive for developers to discover and use
  • Since it's part of Score, it feels less like an anemic domain model

Cons:

  • Still doesn't force developers to use it, but makes the alternative less attractive
This PR implements the option (2) described below with a bit of "hardcoded" instrumentation. The refactoring to better manage state will come in a future PR because of more decoupling needed (`Installable`, `AlertReceiver`, `AlertSender`, all make use of the `InterpretStatus` and such). ## Context In the current design, we execute the interpretation of a score as such: ```rs let score = MyScore {}; let result = score.create_interpret().execute(inventory, topology); ``` The signature being: ```rs trait Score<T: Topology> { fn create_interpret(&self) -> Box<dyn Interpret<T>>; } trait Interpret<T: Topology> { fn execute(&self, inventory: &Inventory, topology: &T) -> Result<InterpretOutcome, InterpretError>; } ``` We can do it whenever we want, wherever we want. Because of this, it is no easy task to control the lifecycle (status) of an Intrepret, without pushing this responsibility back to the developers of scores/interprets. Even if we try to have a "smart" interpret state that handle itself the lifecycle/instrumentation. So after some consideration, here are the 2 options I explored that allows to keep the flexibility of executing interprets whenever, wherever we want as well as composing scores together. ## Possible implementations ### 1. Introduce an `interpret_score` function Plays a similar role to `Maestro::prepare_topology` by "orchestrating" the execution of an `Interpret`: ```rs fn interpret_score(score: Score, topology: &T) -> Result<InterpretState, InterpretError> { let interpret = score.create_interpret(); let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running } let result = interpret.execute(topology); match result => { Ok(outcome) => match outcome { InterpretOutcome::Success { details } => satus.success(details) InterpretOutcome::Noop { details } => satus.noop(details) } Err(err) => status.error(err.message) } result } // example let score = MyScore {}; let result = interpret_score(score, topology); ``` **Pros:** * No need to change existing `Score`/`Interpret` implementations **Cons:** * Adds an extra function * Doesn't force developers to use it ----- ### 2. Add `interpret` directly to `Score` (as a provided method) Similar to proposal (1), the difference is that it's part of the `Score` trait directly, making it easier to discover and use without needing to remember a separate function: ```rs trait Score { fn interpret(&self, topology: &T) -> Result<InterpretState, InterpretError> { let interpret = self.create_interpret(); let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running } let result = interpret.execute(topology); match result => { Ok(outcome) => match outcome { InterpretOutcome::Success { details } => satus.success(details) InterpretOutcome::Noop { details } => satus.noop(details) } Err(err) => status.error(err.message) } result } fn create_interpret(&self) -> impl Interpret; } // example let score = MyScore {}; let result = score.interpret(topology); ``` **Pros:** * No need to change existing `Score`/`Interpret` implementations * Easier and more intuitive for developers to discover and use * Since it's part of `Score`, it feels less like an anemic domain model **Cons:** * Still doesn't force developers to use it, but makes the alternative less attractive
letian added 1 commit 2025-08-07 15:46:17 +00:00
refactor: Interpret score with a provided method on Score
Some checks failed
Run Check Script / check (pull_request) Has been cancelled
Details
2c8f45c44e
letian added 1 commit 2025-08-07 16:47:48 +00:00
instrument interpret state changes (wip)
Some checks failed
Run Check Script / check (pull_request) Has been cancelled
Details
9db475849c
johnride commented 2025-08-08 16:28:46 +00:00
Owner
Copy Link

I 100% agree with the pros as required elements of the design :

No need to change existing Score/Interpret implementations
Easier and more intuitive for developers to discover and use
Since it's part of Score, it feels less like an anemic domain model

However, I think the implementation can be better. Adding orchestration-related logic to the Score feels wrong. I'll try something in a playground when I have an hour to spare.

I 100% agree with the pros as required elements of the design : > No need to change existing Score/Interpret implementations > Easier and more intuitive for developers to discover and use > Since it's part of Score, it feels less like an anemic domain model However, I think the implementation can be better. Adding orchestration-related logic to the Score feels wrong. I'll try something in a playground when I have an hour to spare.
👀 1
letian changed target branch from remove-interpret-status-from-topology to master 2025-08-09 22:52:29 +00:00
letian added 1 commit 2025-08-09 22:53:10 +00:00
Merge branch 'master' into interpret-score
Some checks failed
Run Check Script / check (pull_request) Has been cancelled
Details
ff011ce1d4
letian merged commit 29a261575b into master 2025-08-09 22:56:26 +00:00
letian deleted branch interpret-score 2025-08-09 22:56:27 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
Architecture Decision Record
Good first issue
RFC
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
johnride jvtrudel letian stremblay wjro
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: NationTech/harmony#100
No description provided.
Delete Branch "interpret-score"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?