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

ENH: Add py_trees.behaviours Behaviours, build a functional beams run command #47

Merged
merged 9 commits into from
Sep 25, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Sep 22, 2024

Description

  • Builds a functional beams run command, with options to control
    • tick count
    • logging output
  • Adds the BaseItem subclasses for the built-in py-trees behaviours. This lets us use them in our own trees.
  • Adds lots of tests surrounding the above two
  • Minor touchups
    • refactors the ActionItem class to better support apischema's serialization/deserialization
    • as a result, re-renders the serialized test configs (we should rename these to not be so egg-y)

Also now I'm naturally spelling "Behaviour" with the "u", and the American in me is mad

Motivation and Context

The original goal was to make beams run usable. The side-effect was me being forced to enable beams to specify trees that had nothing to do with EPICS CA. I think these will be useful in the long run.

Tangent: code golf

I initially balked at the idea of writing a whole dataclass for each py_trees.behaviours.Behaviour, when the Behaviour itself already has the type-hinted arguments I need. Could we not just inspect the __init__ signature and dynamically construct the dataclass?

The answer is yes, with a couple of code-inspection/static-analysis caveats:

  • We have to define a set of defaults for all of these, due to dataclass argument ordering (no non-default args after default-provided args)
  • We have to manually adjust new_dataclass.__module__ to point to beams.tree_config
  • These dynamically created dataclasses can be added to the module namespace, but static code analysis will never pick them up.
    • This means these classes would also be undiscoverable by someone who wanted to know what BaseItems we supported (and more importantly the auto-api generated docs)

This last point was the dealbreaker for me, after which I just caved and wrote out the classes manually. (but not until I had already sunk a couple hours on Friday afternoon trying to get it to work)

The code, for the curious

def py_trees_get_tree(self):
    """The get_tree method that will get bound to the dataclass"""
    cls = getattr(py_trees.behaviours, type(self).__name__.removesuffix('Item'))
    kwargs = {}
    for inst_field in fields(self):
        if inst_field.name in ('description',):
            continue
        kwargs[inst_field.name] = getattr(self, inst_field.name)

    return cls(**kwargs)


def generate_behaviour_items():
    items = []
    # _BEHAVIOUR_DEFAULTS holds default values for each field in a Behaviour
    for beh_name, defaults in _BEHAVIOUR_DEFAULTS.items():
        behaviour = getattr(py_trees.behaviours, beh_name)
        sig = inspect.signature(behaviour)
        params = []
        for p_name, p_param in sig.parameters.items():
            if p_name in defaults:
                params.append((p_name, p_param.annotation, defaults[p_name]))

        beh_dataclass = make_dataclass(
            cls_name=f'{beh_name}Item',
            fields=params,
            bases=(BaseItem,),
            namespace={'get_tree': py_trees_get_tree}
        )
        # make the class repr show "beams.tree_config.<name>"  instead of "types.<name>"
        beh_dataclass.__module__ = __name__

        items.append(beh_dataclass)

    return items


for behaviour_item in generate_behaviour_items():
    # bind the new dataclass to this  module's namespace
    globals()[behaviour_item.__name__] = behaviour_item

How Has This Been Tested?

Tests added.

Where Has This Been Documented?

This PR, minimal docstrings have been added so far

image

interactive mode, 5-ticks. Waits for key input between each tick (ctrl+C halts operation)
image

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong marked this pull request as ready for review September 23, 2024 23:34
@tangkong tangkong changed the title ENH: Add py_trees.behaviours Behaviours ENH: Add py_trees.behaviours Behaviours, build a functional beams run command Sep 23, 2024
ZLLentz
ZLLentz previously approved these changes Sep 24, 2024
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

My notes here are super minor, I like how the interactive ticks look and the options you've selected for them.

I have not run this myself.

show_blackboard)
)
tree.setup()
for _ in range(tick_count):
Copy link
Member

Choose a reason for hiding this comment

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

The cli help text says that tick counts <=0 result in continuous ticking, but here it seems like they result in no ticks at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. Well I'm glad I at least communicated my intent instead of forgetting entirely

beams/logging.py Show resolved Hide resolved
@joshc-slac
Copy link
Collaborator

This looks great! Thanks for adding this potent feature, going to play around with it for a second. Off the bat, make test runs successfully but complains about file io on dead workers. I'll see if I can put a one line guard on that
image

@tangkong
Copy link
Contributor Author

I've been seeing the dead workers too, see #49 . It doesn't prevent the tests from passing, fwiw

@joshc-slac
Copy link
Collaborator

I've been seeing the dead workers too, see #49 . It doesn't prevent the tests from passing, fwiw

Gotcha we can keep it to a separate issue, theres a few ways to fix it

joshc-slac
joshc-slac previously approved these changes Sep 24, 2024
beams/bin/run_main.py Outdated Show resolved Hide resolved
Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>
@ZLLentz
Copy link
Member

ZLLentz commented Sep 25, 2024

The pre-commit failure is a new one to me

beams/bin/run_main.py:51:1: C901 'main' is too complex (11)

https://www.flake8rules.com/rules/C901.html

ZLLentz
ZLLentz previously approved these changes Sep 25, 2024
@tangkong tangkong merged commit a41cb6d into pcdshub:master Sep 25, 2024
7 of 11 checks passed
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.

3 participants