Skip to content

Conversation

MattToast
Copy link
Member

Basic implementation of a Record class to keep track of launched jobs. Convert Experiment.start return Records and all other methods to accept records.

@MattToast MattToast added area: job Issues related to job management area: api Issues related to API changes labels Oct 14, 2024
@MattToast MattToast self-assigned this Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (v1.0@879b96e). Learn more about missing BASE report.

Files with missing lines Patch % Lines
smartsim/launchable/job.py 86.66% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             v1.0     #744   +/-   ##
=======================================
  Coverage        ?   59.04%           
=======================================
  Files           ?      142           
  Lines           ?     9140           
  Branches        ?        0           
=======================================
  Hits            ?     5397           
  Misses          ?     3743           
  Partials        ?        0           
Files with missing lines Coverage Δ
smartsim/_core/utils/helpers.py 41.66% <ø> (ø)
smartsim/experiment.py 88.53% <100.00%> (ø)
smartsim/launchable/job.py 93.84% <86.66%> (ø)

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.

Some small suggestions!

:param launch_id: A unique identifier for the launch of the job.
:param job: The job that was launched.
"""
self._id = launch_id
Copy link
Contributor

@amandarichardsonn amandarichardsonn Oct 15, 2024

Choose a reason for hiding this comment

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

Why do you deep copying the Job but not the launch_id here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, the launch_id (at least for now), is just a string that we ask the type checker to treat like a different type, and strings are immutable in python, so a "deepcopy of" and a "reference to" a string are functionally equivalent.

There is actually some special logic in copy.deepcopy so that, by default, it will just return the original reference to a string if its encounter without change.

TL;DR I was lazy and didn't want to spell out "deepcopy". More than willing to add it though if we think that LaunchedJobID might become a more substantive type in future!

@MattToast MattToast changed the base branch from smartsim-refactor to v1.0 October 29, 2024 16:10
@MattToast MattToast force-pushed the start-returns-record branch from 109da7c to c647108 Compare October 29, 2024 16:35
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: job Issues related to job management ignore-for-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants