search

aica-technology / api

AICA API resources
0 stars 0 forks source link

feat(schema): application syntax draft/2-0-0 #162

Closed eeberhard closed 2 months ago

eeberhard commented 3 months ago

Description

Review guidelines

Estimated Time of Review: 90 minutes

Review by commit

Checklist before merging:

eeberhard commented 3 months ago

Example of the new syntax:

# yaml-language-server: $schema=https://raw.githubusercontent.com/aica-technology/api/feat/draft-v2.0.0/docs/static/schemas/draft/2-0-0/application.schema.json

schema: draft/2-0-0

dependencies:
  base: v4.0.0
  packages:
    - { package: "@aica/components/pose-detection", version: "v1.0.0" }
    - { package: "@aica/collections/ur-collection", version: "v3.1.0", constraints: ">= 3.0.0" }

metadata:
  name: My application
  description: Some description of the application
  tags:
    - "motion"
    - "force control"
    - "universal robots"

userdata:
  info: "..."
  website: github.com/eeberhard

on_start:
  load:
    - component: my_component
    - hardware: my_hardware
  sequence:
    start: my_sequence

conditions:
  my_condition:
    display_name: My cool condition
    condition:
      any:
        - component: foo
          state: active
        - controller: foo
          hardware: bar
          predicate: baz
    events:
      unload:
        component: foo

sequences:
  my_sequence:
    display_name: My cool sequence
    steps:
      - delay: 10
      - check:
          condition:
            condition: my_condition
          else:
            load:
              component: foo
      - check:
          condition:
            component: foo
            state: active
          wait_forever: true
      - check:
          condition:
            sequence: foo
            state: active
          timeout: 10
          else:
            unload:
              component: foo

hardware:
  my_hardware:
    urdf: foo
    rate: 12.5    
    rate_tolerance: 0.99
    strict: true
    events:
      transitions:
        on_load:
          load:
            controller: my_controller
            hardware: my_hardware
    controllers:
      my_controller:
        plugin: my_controller/Foo
        events:
          transitions:
            on_load:
              switch_controllers:
                hardware: my_hardware
                activate: my_controller
          predicates:
            is_stable:
              sequence:
                start: my_sequence

components:
  foo:
    component: my_package::Foo
    parameters:
      a: foo
      b: bar
    inputs:
      my_input: /foo/my_input
    outputs:
      my_output: /foo/my_output
    events:
      transitions:
        on_configure:
          lifecycle: activate
      predicates:
        is_in_range:
          unload:
            component: foo

graph:
  positions:
    on_start:
      x: 20
      y: 40
    conditions:
      my_condition:
        x: 0
        y: 0
    sequences:
      my_sequence:
        x: 0
        y: 0
    components:
      foo:
        x: 0
        y: 0
    hardware:
      my_hardware:
        x: 0
        y: 0
eeberhard commented 3 months ago

Thanks for the feedback! Actually a lot of your points come down to this new way of triggering events from state transitions.

now under events we need to have an additional property transitions or predicates

As you said, we need to distinguish these two types of event drivers. I considered a flatter structure like this but I didn't like it as much:

my_component:
  component: my_component::Foo
  transition_events:  # or just `transitions:`
    on_load: ...
  predicate_events:  # or just `predicates:`
    is_in_range: ...

on load, we activate the controller but how can you be sure that the controller is already loaded..

Predicates trigger events on any rising edge. But the intention with state transition events is that they trigger an event iff the specific transition is completed. In the implementation of transition events, I would only trigger on_activate events after the component / controller / etc has actually been activated. Because the underlying engine already manages and monitors the states of all components, we can actually avoid race conditions with this pattern better than the current implementation.

I can see the confusion because in the current implementation, on_load is a fake predicate that is actually triggered asynchronously with component loading and does not actually guarantee that the component has been fully constructed. The other potentially confusing point is that a transition like on_activate looks a lot like the transition callback function in the modulo component SDK (which is partly intentional), but in the SDK that function is called during the transition, not after. On that note...

Why do we go from is_active to on_activate?

I wanted to use a recognizable and consistent "transition callback" language similar to the component SDK. The previous predicate formulation is_active triggered the event every time the component state changed from not active to active. For that particular transition it is equivalent. But now consider the is_inactive predicate, which could be triggered both after configuring or after deactivating. So the name of state transitions events should reflect the triggering transition like on_configure and on_deactivate.

You might call it after_load, after_configure, ... but to me the on_ feels right.

Are those really all the states [...] about controllers? no inactive or configured?

Actually controllers have full lifecycle states but are automatically configured and go to inactive when loaded. The only transitions we really control are loading, activating, deactivating and unloading (and thereby the only primary states the controller stays in are unloaded, inactive or active).

Part of the reason of this being a draft is to just try the implementation and see what states actually make sense to control and expose for hardware and controllers.

domire8 commented 3 months ago

As you said, we need to distinguish these two types of event drivers. I considered a flatter structure like this but I didn't like it as much

Agreed.

Because the underlying engine already manages and monitors the states of all components, we can actually avoid race conditions with this pattern better than the current implementation.

Absolutely, and this will constitute a huge improvement obviously. I know all that but that underlying mechanism might not fully transparent to the user.

I wanted to use a recognizable and consistent "transition callback" language similar to the component SDK

I agree about similar language but part of me still think it's too similar with the on_activate. A user that writes a component with the SDK has to know what to put in the on_activate_callback and then we tell him that if you put on_activate into the YAML app, it will be triggered after on activate. I don't know...

after_ just doesn't look nice as a prefix :smile: I think we start with this version and see how we feel about it later

yrh012 commented 2 months ago

Very cool stuff! 🔥

One thing that we should consider is enforcing the jsonschema to not allow a list of load/lifecycle events in a given sequence step.

domire8 commented 2 months ago

One thing that we should consider is enforcing the jsonschema to not allow a list of load/lifecycle events in a given sequence step.

Can you elaborate with an example? Is that something that's already not correct in the current schema or did we miss this in this PR?

domire8 commented 2 months ago

This PR also addresses issues

yrh012 commented 2 months ago

One thing that we should consider is enforcing the jsonschema to not allow a list of load/lifecycle events in a given sequence step.

Can you elaborate with an example? Is that something that's already not correct in the current schema or did we miss this in this PR?

Here is an example with a small sequence that loads a list of components. In the current implementation, this is allowed in the schema but fails at execution indicating invalid load payload.

sequences:
  prepare_components:
    # load the components
    - load: 
        - component: assembled_frame_broadcaster
        - component: approach_frame_broadcaster
        - component: approach_tf_to_signal

when changed to a load step for each component, it works as expected. 

sequences:
  prepare_components:
    # load the components
    - load: 
        component: assembled_frame_broadcaster
    - load: 
        component: approach_frame_broadcaster
    - load: 
        component: approach_tf_to_signal
eeberhard commented 2 months ago

This PR also addresses issues [...]

Thanks @domire8 !

One thing that we should consider is enforcing the jsonschema to not allow a list of load/lifecycle events in a given sequence step

I think this is something to discuss further in the parent issue #147, but as @yrh012 points out, the current schema allows a sequence step to contain a "multi-event structure". However, in the current backend implementation (using base v3.4.0 for example), a sequence step can only support a single event.

This basically means there is a discrepancy between the current schema and implementation, and the observation of that is more of a general bug / issue report. However the relevant thing to discuss for the point of syntax 2.0 is whether to [1] change the schema to match the implementation (restrict a sequence step to a single event) or [2] do not change the schema and instead fix the implementation to support a multi-event structure on a sequence step. The option [1] is a breaking syntax change and so would need t be part of 2.0.

eeberhard commented 2 months ago

As discussed, I will merge this now such that we can start to roll out the draft/2-0-0 implementations. We can then make additional changes based on the comments as discussed before we officially release it.