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

Change default path for entities #533

Merged
merged 33 commits into from
Apr 17, 2024

Conversation

amandarichardsonn
Copy link
Contributor

@amandarichardsonn amandarichardsonn commented Mar 26, 2024

This PR makes changes to the default path for SS entities. New default path is exp_path/entity_name/.

@amandarichardsonn amandarichardsonn marked this pull request as draft March 26, 2024 21:05
@amandarichardsonn amandarichardsonn changed the title draft Change default path for entities Mar 26, 2024
@amandarichardsonn amandarichardsonn added area: python Issues related to the Python Client repo: smartsim Issues related to SmartSim infrastructure library type: feature Issues that include feature request or feature idea labels Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@4c9643c). Click here to learn what that means.

❗ Current head 23ed532 differs from pull request most recent head 273b973. Consider uploading reports for the commit 273b973 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #533   +/-   ##
==========================================
  Coverage           ?   71.03%           
==========================================
  Files              ?       65           
  Lines              ?     4544           
  Branches           ?        0           
==========================================
  Hits               ?     3228           
  Misses             ?     1316           
  Partials           ?        0           
Files Coverage Δ
smartsim/database/orchestrator.py 51.96% <ø> (ø)
smartsim/entity/ensemble.py 64.44% <100.00%> (ø)
smartsim/entity/model.py 64.05% <100.00%> (ø)
smartsim/experiment.py 70.41% <100.00%> (ø)

@amandarichardsonn amandarichardsonn marked this pull request as ready for review April 1, 2024 15:54
@mellis13 mellis13 requested a review from ashao April 1, 2024 18:05
Copy link
Collaborator

@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.

Looking pretty good. See the above comments for specific places but overall here are the items to address:

  • Try to avoid mixing using os.path and pathlib where possible
  • See if you can figure out a way to monkeypatch exp.start so that you can avoid actually having to run the database and other entities

One task to address that's outside of the inline comments:

  • Go through the rest of the tests and make sure that we're not unnecessarily setting their paths anymore

@amandarichardsonn amandarichardsonn requested a review from ashao April 8, 2024 22:35
Copy link
Collaborator

@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.

Thanks for addressing my previous comments. I had a handful of minor points this time around, but nothing substantive. Good job!

@amandarichardsonn amandarichardsonn requested a review from ashao April 17, 2024 15:30
Copy link
Collaborator

@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.

Looks good. Thanks for improvements!

@amandarichardsonn amandarichardsonn merged commit f5beb41 into CrayLabs:develop Apr 17, 2024
34 checks passed
@amandarichardsonn amandarichardsonn deleted the paths branch April 17, 2024 21:27
@amandarichardsonn amandarichardsonn added type: usability Issues related to ease of use area: api Issues related to API changes labels Apr 25, 2024
ankona pushed a commit to ankona/SmartSim that referenced this pull request May 6, 2024
This PR makes changes to the default path for SS entities. New default
path is `exp_path/entity_name/`. A path argument has also been added to
create_ensemble and create_model.

[ reviewed by @ashao @mellis13 ]
[ committed by @amandarichardsonn ]
ankona pushed a commit to ankona/SmartSim that referenced this pull request May 7, 2024
This PR makes changes to the default path for SS entities. New default
path is `exp_path/entity_name/`. A path argument has also been added to
create_ensemble and create_model.

[ reviewed by @ashao @mellis13 ]
[ committed by @amandarichardsonn ]
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: python Issues related to the Python Client repo: smartsim Issues related to SmartSim infrastructure library type: feature Issues that include feature request or feature idea type: usability Issues related to ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants