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

Support flat storage for single-state machines #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nickswalker
Copy link

@nickswalker nickswalker commented Feb 8, 2020

In our use case, we'll create a lot of library states which are stand alone ExecutionStates tied to our robot's capabilities. As it stands, we'd have a folder structure like this:

common_states/
    move_to_named_poi/
        state_machine.json
        meta_data.json
        move_to_named_poi_MOVE_TO_NAMED_POI/
            script.py
            code_data.json
            meta_data.json

This is pretty heavy wrapping for a single script file, so we'd greatly prefer a structure like this:

common_states/
    move_to_named_poi/
        state_machine.json
        meta_data.json
        script.py
        code_data.json

This PR has changes to support this layout. The loading code evaluates whether a state machine is of this type by looking for a state in the current folder whenever "root_state_storage_id" is absent. Saving will adopt this format if the state machine contains exactly one state and the state is an ExecutionState.

Please do feel free to recommend alternative implementations. Just wanted to start a discussion about this as a possible convenience.


Please make sure that you can tick the following items at latest before
merging the pull request:

  • added description what has been changed and why
  • if appropriate, include changes into the changelog
  • if appropriate, adapt/extend the documentation
  • as external contributor, you need to sign the Contributor License Agreement (CLA), which can be found in the root directory of the repository

For reviewers:

  • after merging the pull request, check if everything looks like expected (especially the changelog)

In this common use case, where a library state-machine consists
of just one ExecutionState, the regular folder structure results in
a lot of duplicated information, and is nested an extra level unnecessarily.
These changes these kinds of state machines to be stored in a single directory
with no subdirectories. Storage code evaluates whether a state machine is of this type
by noticing the absence of "root_state_storage_id" and looking for a state in the current
folder in this case. Saving will adopt this format if the state machine contains exactly one
state and the state is an ExecutionState
@franzlst
Copy link
Member

Hi @nickswalker, thank you very much for your pull request!

Before considering it for merging, could you please motivate the change a bit more? Personally, I don't see the point why we should add ~35 LOC, just to save one file and one folder in the data structure for a specific (yet often occuring) kind of state machine.

Do you edit a state machine often directly in the file system? Do you often browse through the file system?

@nickswalker
Copy link
Author

Sure! Here are what I see as the main benefits:

  • We would like to enforce a separation between the structure of our state machine and the implementation of individual states, so as to encourage folks to build generic and reusable skills. As a mechanism for this, in parts of our codebase, we will encourage people to build state machines that are either composed entirely of library states (pure structure), or entirely of a single execution state (pure implementation). In our use case, having single-state state machines stored flat makes it easier for us to distinguish states that provide implementation.
  • To use another RoboCup team as an example, rtlions' RAFCON repo contains 105 state machines, of which 60 are single-state state-machines. So, for users like us, single-state machines are the expected majority of states, so any possible streamlining merits consideration. They're tracking 60 files + 60 folders worth of boilerplate. This change would cut the listing footprint of those states by a third.
  • We plan to develop library states through external editors so we can leverage existing tooling (debuggers, formatting tools, VCS integration). Flattening single-state state machines makes it easier for us to reach into a state to start editing by reducing folder twirls and making the file view more compact when working across a couple of states at a time. Here's a screen shot of what having three of rtlions' states open looks like in CLion:
    Screenshot from 2020-02-12 12-57-29
    It's very easy to get lost in all of the structure, so every bit of noise reduction helps.
  • We're largely reliant on GitHub for code review, so reducing nesting, makes it easier to browse code through GitHub's file browser, which requires a click and a load for each level of hierarchy. Looking beyond our team, we'd like other teams to be able to look at our implementation easily, and they'll primarily use the GitHub file browser to do that too. To draw another comparison for browsability and developer ease, consider what it's like to flip through a repo of smach library states. Here's another team's package of robot smach states.
  • (Minor) more compact diffs for new states reduce noise in code review.

All in all, the savings that's achievable with minor changes seems like a good value to me 😄

@franzlst
Copy link
Member

These are indeed all very good points.

I am even thinking about applying this flat structure to all state machines, independent of the type of the root state. I will discuss this with my colleague.

In general, I think we will release this in a new minor version.

@franzlst
Copy link
Member

I talked to @sebastian-brunner and we agreed upon to entirely remove the first file system layer, as you suggest for ExecutionStates.

If this OK to you, I would like to continue implementing it. If you sign the CLA ("as external contributor, you need to sign the Contributor License Agreement (CLA), which can be found in the root directory of the repository"), I will implement the changes based on your pull request. Otherwise, I will start a new branch.

The idea is to move core_data.json, semantic_data.json and meta_data.json to the root folder, rename meta_data.json of the state machine (which is currently unsed) to sm_meta_data.json and remove the root_state_storage_id key from statemachine.json.

Next week on Wednesday, we will make the last patch release of the 0.14 version. After this, we try remove some code intended for backward compatibility. Then we will continue with this feature request. All this will then be part of a new minor version.

@sebastian-brunner
Copy link
Member

Thanks for your pr @nickswalker. I like the idea of building state machines that are either composed entirely of library states, or entirely of a single execution state. To really enforce no structure in a ExecutionState you have to make sure that nobody implements (in the worst case) whole state machines in pure python code in a single ExecutionState. Probably you have guidelines for that!?

On top of that I like the idea of reducing the folder nesting, which makes the storage format more compact and thus ease versioning!

@franzlst
Copy link
Member

I started to continue with your change on branch feat/flatter_hierarchy. New state machines are now always saved with one hierarchy level less.

Handling of existing state machines is not yet working properly!

@nickswalker: Feel free to test this!
@sebastian-brunner: You also might already have a look at it

@nickswalker
Copy link
Author

Notes from brief testing:

  • sm_metadata.json seems like a file that will be empty in a lot of common usage. Could it be omitted whenever it has an empty dict, like in Suggestion: Do not store semantic data if not available #21?
  • Silent conversion of the format when resaving worked fine but did leave behind the old root state folder for the single executionstate case. This didn't happen for the root hierarchystate case.

@franzlst
Copy link
Member

franzlst commented Mar 3, 2020

sm_metadata.json seems like a file that will be empty in a lot of common usage. Could it be omitted whenever it has an empty dict, like in #21?

Yes, this is a good idea.

Silent conversion of the format when resaving worked fine but did leave behind the old root state folder for the single executionstate case. This didn't happen for the root hierarchystate case.

Resaving old state machines has not been handled yet in this pull request, but is on the agenda.

We are currently quite busy due to upcoming events, so I cannot tell you when this will be finished. It will be part of a new minor release, but for this release we also want to remove some old code, which also takes time.

You should be able to work with this branch and later use the new minor version without problems (as long as you do not resave old state machines).

sebastian-brunner pushed a commit to AgileRobotsAG/RAFCON that referenced this pull request Nov 26, 2021
…lop'

do not create already existing transitions

Closes DLR-RM#31

See merge request dev/sys/tci/rafcon!27
@JohannesErnst
Copy link
Collaborator

JohannesErnst commented Jan 29, 2024

The previous mentioned commit (AgileRobotsAG@5b589ba) states to close this open issue. However, I couldn't identify any of the proposed changes mentioned in this pull request to be added with the previously mentioned commit.
Therefore, this issue remains for now...

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.

4 participants