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

Replace TaggedFilesHierarchy with os.walk and implement configure_directory entrypoint #695

Merged

Conversation

amandarichardsonn
Copy link
Contributor

@amandarichardsonn amandarichardsonn commented Sep 5, 2024

This PR adds a configure_directory entry point, as well as tests. It also removes TaggedFilesHierarchy and replaces it with os.walk. Finally, the Generator tests have been refactored.

@amandarichardsonn amandarichardsonn changed the title File ops Replace TaggedFilesHierarchy with os.walk and implement configure_directory entrypoint Sep 5, 2024
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.24%. Comparing base (cce16e6) to head (dfbb53f).
Report is 16 commits behind head on smartsim-refactor.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                  @@
##           smartsim-refactor     #695      +/-   ##
=====================================================
+ Coverage              40.45%   46.24%   +5.78%     
=====================================================
  Files                    110      107       -3     
  Lines                   7326     6446     -880     
=====================================================
+ Hits                    2964     2981      +17     
+ Misses                  4362     3465     -897     
Files with missing lines Coverage Δ
smartsim/_core/commands/command_list.py 97.61% <100.00%> (ø)
smartsim/_core/generation/generator.py 100.00% <100.00%> (+2.22%) ⬆️
smartsim/entity/__init__.py 100.00% <ø> (ø)
smartsim/entity/files.py 71.73% <ø> (-3.53%) ⬇️
smartsim/experiment.py 86.52% <100.00%> (+4.97%) ⬆️

... and 8 files with indirect coverage changes

Copy link
Contributor

@juliaputko juliaputko left a comment

Choose a reason for hiding this comment

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

1 minor comment, otherwise lgtm!!

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.

Just some initial feedback while the CI gets worked out. So far looks good!

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 some general comments to get the review process started.

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 comments/questions.

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.

Only one small item but otherwise I think this is ready to merge and other items can be addressed as the user API for attaching files is updating. Thanks!

@amandarichardsonn amandarichardsonn merged commit 4d9ab27 into CrayLabs:smartsim-refactor Sep 27, 2024
34 of 35 checks passed
@amandarichardsonn amandarichardsonn deleted the file_ops branch September 27, 2024 19:29
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.

4 participants