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 FeatureStore descriptor to tensor & model keys #633

Merged
merged 49 commits into from
Aug 7, 2024

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Jul 16, 2024

  • Enables using multiple feature stores by enhancing the existing tensor/model-key classes to include the feature store descriptor.
  • Update the EnvironmentConfigLoader to retrieve multiple feature stores from environment using the prior key as a prefix to query with
  • Minor (lift & shift) refactor of top-level functions in worker manager module to reduce number of touch-points for converting to FeatureStoreKey from capnproto type
    • now, only worker.py deals with this conversion.

@ankona ankona force-pushed the 736-2 branch 2 times, most recently from 6c4320d to 1d110ae Compare July 16, 2024 20:15
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 27.11864% with 86 lines in your changes missing coverage. Please review.

Please upload report for BASE (mli-feature@263e3c7). Learn more about missing BASE report.

Files Patch % Lines
smartsim/_core/mli/infrastructure/worker/worker.py 37.03% 34 Missing ⚠️
...tsim/_core/mli/infrastructure/environmentloader.py 0.00% 33 Missing ⚠️
...e/mli/infrastructure/storage/dragonfeaturestore.py 0.00% 19 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff               @@
##             mli-feature     #633   +/-   ##
==============================================
  Coverage               ?   65.21%           
==============================================
  Files                  ?       97           
  Lines                  ?     6840           
  Branches               ?        0           
==============================================
  Hits                   ?     4461           
  Misses                 ?     2379           
  Partials               ?        0           
Files Coverage Δ
smartsim/_core/launcher/dragon/dragonBackend.py 35.54% <ø> (ø)
smartsim/_core/mli/comm/channel/channel.py 66.66% <ø> (ø)
...m/_core/mli/infrastructure/storage/featurestore.py 100.00% <100.00%> (ø)
smartsim/_core/mli/message_handler.py 75.59% <100.00%> (ø)
...e/mli/infrastructure/storage/dragonfeaturestore.py 0.00% <0.00%> (ø)
...tsim/_core/mli/infrastructure/environmentloader.py 0.00% <0.00%> (ø)
smartsim/_core/mli/infrastructure/worker/worker.py 59.83% <37.03%> (ø)

@ankona ankona changed the title Add file system descriptor to tensor & model keys Add FeatureStore descriptor to tensor & model keys Jul 17, 2024
@ankona ankona added area: ML Issues related to SmartSim ML classes and utilities repo: smartsim Issues related to SmartSim infrastructure library area: settings Issues related to Batch or Run settings labels Jul 18, 2024
@ankona ankona force-pushed the 736-2 branch 2 times, most recently from 93d2380 to bf32f12 Compare July 18, 2024 22:23
@ankona ankona marked this pull request as ready for review July 18, 2024 22:23
@ankona ankona requested review from AlyssaCote, al-rigazzi and mellis13 and removed request for al-rigazzi July 18, 2024 23:20
Copy link
Contributor

@AlyssaCote AlyssaCote left a comment

Choose a reason for hiding this comment

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

Loving these changes!! Really my only request is that we update files pertaining to the mli_driver so that we can run it without it crashing and burning.

Copy link
Contributor

@mellis13 mellis13 left a comment

Choose a reason for hiding this comment

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

Just a couple of initial comments/questions

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.

Per our recent conversation, I am sorry you had to tie up so many loose ends created by the hacky solutions. I'll review the PR again once previous comments are addressed.

@ankona ankona force-pushed the 736-2 branch 7 times, most recently from 36e0b1a to 19d35ab Compare July 29, 2024 14:35
@AlyssaCote AlyssaCote self-requested a review July 30, 2024 22:03
@ankona ankona requested a review from mellis13 July 31, 2024 17:48
Copy link
Contributor

@AlyssaCote AlyssaCote left a comment

Choose a reason for hiding this comment

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

Love the changes! Just a few small cleanup comments.

@ankona ankona force-pushed the 736-2 branch 4 times, most recently from cf50cc1 to 108046e Compare August 3, 2024 00:05
@ankona ankona merged commit 0453b8b into CrayLabs:mli-feature Aug 7, 2024
24 of 42 checks passed
@ankona ankona deleted the 736-2 branch September 25, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ML Issues related to SmartSim ML classes and utilities area: settings Issues related to Batch or Run settings repo: smartsim Issues related to SmartSim infrastructure library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants