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

Add memory & conn collector, collector manager, tests #460

Merged
merged 199 commits into from
Mar 28, 2024

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Jan 19, 2024

New features

  • Creates and integrates metric collection into the telemetry monitor using the new CollectorManager. Metrics are written using the new FileSink class

Included collectors:

  • DbMemoryCollector
  • DbConnectionCollector

Updated features

  • Switch basic experiment tracking telemetry to default to on.
  • Improve telemetry monitor logging.
  • Create telemetry subpackage at smartsim._core.utils.telemetry.
  • Refactor telemetry monitor entrypoint.

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: Patch coverage is 90.89616% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 90.65%. Comparing base (4b35cc9) to head (104d189).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #460      +/-   ##
===========================================
- Coverage    90.72%   90.65%   -0.08%     
===========================================
  Files           60       65       +5     
  Lines         3818     4494     +676     
===========================================
+ Hits          3464     4074     +610     
- Misses         354      420      +66     
Files Coverage Δ
smartsim/_core/control/controller.py 85.91% <ø> (-0.04%) ⬇️
smartsim/_core/control/manifest.py 96.52% <ø> (ø)
smartsim/_core/launcher/step/step.py 100.00% <100.00%> (ø)
smartsim/_core/utils/helpers.py 91.96% <100.00%> (ø)
smartsim/_core/utils/serialize.py 100.00% <ø> (ø)
smartsim/_core/utils/telemetry/sink.py 100.00% <100.00%> (ø)
smartsim/database/orchestrator.py 87.00% <100.00%> (+0.15%) ⬆️
smartsim/entity/__init__.py 100.00% <100.00%> (ø)
smartsim/entity/entity.py 96.66% <100.00%> (+4.35%) ⬆️
smartsim/experiment.py 85.80% <100.00%> (+0.09%) ⬆️
... and 6 more

... and 1 file with indirect coverage changes

@MattToast MattToast self-requested a review January 25, 2024 17:53
@ankona ankona requested a review from al-rigazzi January 29, 2024 18:36
@ankona ankona marked this pull request as ready for review January 30, 2024 06:11
@ankona ankona force-pushed the 612 branch 5 times, most recently from b6198a4 to 85a303a Compare January 31, 2024 19:08
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Okay! Still making my way through this PR, but so far looks very impressive. I commented on a lot of the "low hanging fruit" so you at least have something to go off of while you're sorting out why the CI is failing!

I promise to have a more complete review your way ASAP!!

Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Couple minor questions, overall looks very good

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Couple of other last minute, pedantic things I noticed while reviewing the tests!

MattToast added a commit to MattToast/SmartSim that referenced this pull request Feb 7, 2024
Sets the default value of the "enable telemetry" flag to on. Currently
this will enable telemetry system wide until finer grain control can be
established with CrayLabs#460
@MattToast MattToast mentioned this pull request Feb 7, 2024
3 tasks
Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Nice work! I like the overall structure. I have some comments/feedback that are geared towards future-proofing complexity and scale.

MattToast added a commit that referenced this pull request Feb 7, 2024
This PR makes several patch changes to prepare for a SmartSim release
including:

- Set the default value of the "enable telemetry" flag to on.
Currently this will enable telemetry system wide until finer grain
control can be established with #460
- Bump the output `manifest.json` version number to match that of
`smartdashboard`
- Pin a watchdog version to avoid build errors


[ committed by @MattToast @ankona ]
[ reviewed by @ankona ]

---------

Co-authored-by: Christopher McBride <christopher.mcbride@gmail.com>
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Two quick comments

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

I have some truly menial docstr/typing comments, but otherwise approval still stands!

@ankona ankona merged commit 13d0302 into CrayLabs:develop Mar 28, 2024
44 checks passed
@ankona ankona deleted the 612 branch June 3, 2024 21:04
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.

5 participants