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

TelemetryProducer calls state change hook methods regardless of state change #483

Open
3 tasks
MattToast opened this issue Feb 9, 2024 · 0 comments
Open
3 tasks
Labels
area: api Issues related to API changes area: telemetry Issues related to dashboard telemetry bug: minor A minor bug type: refactor Issues focused on refactoring existing code

Comments

@MattToast
Copy link
Member

MattToast commented Feb 9, 2024

This is a breakout ticket from #460 regarding this review comment

Description

As part of #460 a TelemetryProducer class was added to allow for more fine grain control over which specific SmartSim entity instances should be producing telemetry output. To do this, entities simply instance an TelemetryProducer as an instance attr and then itself if just a class to track the state of a flag.

In order for entities to be able to react to a change in the flag output, developers can subclass the TelemetryProducer and override two hook methods: on_enable and on_disable. These methods were intended to be called when the state of the flag changed. However the current implementation calls them on each .enable()/.disable() call respectively, regardless of if the flag state change.

This implementation here is fairly trivial, but was leading to to some failures in #460 that we did not want to lump into that PR. They should be investigated on their own as it could be emblematic of a larger existing problem already existing in the SmartSim codebase.

How to Reproduce

Consider the following repl output:

>>> from smartsim.entity.entity import TelemetryProducer
>>>
>>> class MyProducer(TelemetryProducer):
...     def on_enable(self):
...         print("this will print even if already enabled!")
...
>>> mp = MyProducer(False)
>>> mp.enable()
this will print even if already enabled!
>>> mp.is_enabled
True
>>> mp.enable()
this will print even if already enabled!
>>> # ^^ Ideally this should not be printed

Expected Behavior

The on_{enable,disable} hooks should only be called when the _is_on flag changes states:

>>> mp = MyProducer(False)
>>> mp.enable()
this will print even if already enabled!
>>> mp.is_enabled
True
>>> mp.enable()
>>> # Note: no text to stdout!

Acceptance Criteria

  • Implement the functionality as described above
  • Add a test(s) to ensure TelemetryProducer derivatives behave as expected
  • Investigate/fix the errors coming from the test suite
@MattToast MattToast added type: refactor Issues focused on refactoring existing code area: api Issues related to API changes bug: minor A minor bug area: telemetry Issues related to dashboard telemetry labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API changes area: telemetry Issues related to dashboard telemetry bug: minor A minor bug type: refactor Issues focused on refactoring existing code
Projects
None yet
Development

No branches or pull requests

1 participant