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

Merge core refactor into v1.0 feature branch #756

Merged
merged 118 commits into from
Oct 28, 2024

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Oct 21, 2024

Combine the core-refactor feature branch with mli-feature in v1.0 branch

amandarichardsonn and others added 30 commits May 2, 2024 16:55
Move the test suite used for SmartSim v0.X.Y to a `tests/_legacy` dir.
This directory is explicitly skipped during test collection as these
tests are not expected to pass as SmartSim transitions to a new user
facing and core API.

Additional changes to CI to make sure the feature branch continues
to pass while in a chaotic development state.

[ committed by @MattToast ]
[ reviewed by @amandarichardsonn ]
Fixed inconsistency when adding run arguments N, nodes, or ntasks, to a RunSettings object with leading `-` characters.

 [ committed by @juliaputko ]
 [ reviewed by @amandarichardsonn  ]
BaseJobGroup, JobGroup and ColocatedJobGroup skeletons added.

[ Committed by @amandarichardsonn ]
[ Reviewed by @juliaputko ]
This PR adds the Capnproto schemas and initial MessageHandler class and
tests.
This PR contains an ML worker manager MVP. The worker manager executes a
single-threaded version of the planned ML pipeline for a single worker
instance.

[ committed by @ankona ]
[ approved by @mellis13 ]
This PR removes `device` from the schemas, MessageHandler, and tests.
Add `Model` schema with model metadata.

[ committed by @AlyssaCote ]
[ approved by @ankona ]
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 82.58883% with 343 lines in your changes missing coverage. Please review.

Please upload report for BASE (v1.0@8a19dee). Learn more about missing BASE report.

Files with missing lines Patch % Lines
smartsim/database/orchestrator.py 18.75% 52 Missing ⚠️
smartsim/_core/control/manifest.py 36.36% 28 Missing ⚠️
smartsim/_core/utils/helpers.py 62.16% 28 Missing ⚠️
smartsim/_core/launcher/dragon/dragon_launcher.py 53.19% 22 Missing ⚠️
smartsim/_core/utils/serialize.py 13.33% 13 Missing ⚠️
smartsim/_core/launcher/dragon/dragon_connector.py 21.42% 11 Missing ⚠️
smartsim/_core/launcher/step/step.py 31.25% 11 Missing ⚠️
smartsim/_core/utils/telemetry/collector.py 21.42% 11 Missing ⚠️
smartsim/_core/launcher/step/mpi_step.py 37.50% 10 Missing ⚠️
smartsim/settings/arguments/batch_arguments.py 74.28% 9 Missing ⚠️
... and 39 more
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             v1.0     #756   +/-   ##
=======================================
  Coverage        ?   58.96%           
=======================================
  Files           ?      142           
  Lines           ?     9126           
  Branches        ?        0           
=======================================
  Hits            ?     5381           
  Misses          ?     3745           
  Partials        ?        0           
Files with missing lines Coverage Δ
smartsim/_core/__init__.py 100.00% <100.00%> (ø)
smartsim/_core/arguments/shell.py 100.00% <100.00%> (ø)
smartsim/_core/commands/__init__.py 100.00% <100.00%> (ø)
smartsim/_core/commands/launch_commands.py 100.00% <100.00%> (ø)
smartsim/_core/config/config.py 81.69% <100.00%> (ø)
smartsim/_core/control/__init__.py 100.00% <ø> (ø)
smartsim/_core/control/interval.py 100.00% <100.00%> (ø)
smartsim/_core/generation/generator.py 100.00% <100.00%> (ø)
smartsim/_core/generation/operations/operations.py 100.00% <100.00%> (ø)
...rtsim/_core/generation/operations/utils/helpers.py 100.00% <100.00%> (ø)
... and 85 more

@ankona ankona force-pushed the v1.0-merge-core branch 2 times, most recently from 78a3a33 to bfc36e3 Compare October 21, 2024 22:49
doc/changelog.md Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
doc/changelog.md Show resolved Hide resolved
.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
smartsim/experiment.py Show resolved Hide resolved
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 super small linting nits, but otherwise it looks like the majority of the smartsim-core work is here and in-tack! Thanks for handling this super large merge!!

.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
.github/workflows/build_docs.yml Outdated Show resolved Hide resolved
.github/workflows/changelog.yml Outdated Show resolved Hide resolved
.github/workflows/run_tests.yml Outdated Show resolved Hide resolved
smartsim/_core/_cli/validate.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/alps_step.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/alps_step.py Outdated Show resolved Hide resolved
smartsim/settings/common.py Outdated Show resolved Hide resolved
Comment on lines +42 to +56
def test_unpack_iterates_over_nested_jobs_in_expected_order(wlmutils):
launch_settings = LaunchSettings(wlmutils.get_test_launcher())
app = Application("app_name", exe="python")
job_1 = Job(app, launch_settings)
job_2 = Job(app, launch_settings)
job_3 = Job(app, launch_settings)
job_4 = Job(app, launch_settings)
job_5 = Job(app, launch_settings)

assert (
[job_1, job_2, job_3, job_4, job_5]
== list(unpack([job_1, [job_2, job_3], job_4, [job_5]]))
== list(unpack([job_1, job_2, [job_3, job_4], job_5]))
== list(unpack([job_1, [job_2, [job_3, job_4], job_5]]))
)
Copy link
Member

Choose a reason for hiding this comment

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

@MattToast this test looks like it is new for the V1 API (and most of the tests in this file look like they are still valid).

TODO: Make a ticket to move valid tests from this file out of tests/_legacy

tests/_legacy/test_run_settings.py Show resolved Hide resolved
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.

LGTM!! Thanks for getting us down to one really big feature branch instead of two really big (and constantly diverging) feature branches lol

Copy link
Contributor

@amandarichardsonn amandarichardsonn left a comment

Choose a reason for hiding this comment

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

Hey bro, gnarly job! Keep shredding and stay stoked, man! LGTM 🌊🏄‍♂️

@ankona ankona changed the title V1.0 merge core refactor Merge core refactor into v1.0 feature branch Oct 24, 2024
@ankona ankona merged commit 5bdafc5 into CrayLabs:v1.0 Oct 28, 2024
84 of 85 checks passed
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.

6 participants