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

Refactor of Generation class and inject path into Launch process #650

Merged

Conversation

amandarichardsonn
Copy link
Contributor

No description provided.

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.

Some more philosophy question I wanted to throw your way before I started looking at this too in depth. Let me know what you think!

Otherwise, I think this starting to look about ready to me!

smartsim/_core/entrypoints/file_operations.py Outdated Show resolved Hide resolved
smartsim/launchable/job.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
@@ -179,13 +181,23 @@ def start(self, *jobs: Job) -> tuple[LaunchedJobID, ...]:
jobs that can be used to query or alter the status of that
particular execution of the job.
"""
return self._dispatch(dispatch.DEFAULT_DISPATCHER, *jobs)
run_id = datetime.datetime.now().replace(microsecond=0).isoformat()
"""Create the run id"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for function variables it may be more natural for the comments to come before the variable if they are going to be added. I'm not seeing in vscode that they render as a comment

Copy link
Member

Choose a reason for hiding this comment

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

If we are going to put these comments ahead of the variable as non-docstrings, can we make these look like traditional comments?

Suggested change
"""Create the run id"""
# Create the run id

In theory it makes saves us a no-op instruction at runtime, but in practice I just think it looks nicer, hahaha

smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
"""
job_type = f"{job.__class__.__name__.lower()}s"
job_path = pathlib.Path(root) / f"{job_type}/{job.name}-{job_index}"
job_path.mkdir(exist_ok=True, parents=True)
Copy link
Contributor

@mellis13 mellis13 Aug 8, 2024

Choose a reason for hiding this comment

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

I think we need to make a ticket to inject a launcher in here. I think we want to use the entrypoints to enable remote jobs.

smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/experiment.py Outdated Show resolved Hide resolved
smartsim/launchable/job.py Outdated Show resolved Hide resolved
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.

Thanks for working on this -- just a couple of questions/comments

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.

A couple of small docstring nits, and one outstanding "TODO" style comment, but definitely nothing worth holding approval up over.

LGTM!! Thanks for all the hard work on this one!

smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
smartsim/_core/generation/generator.py Outdated Show resolved Hide resolved
@@ -179,13 +181,23 @@ def start(self, *jobs: Job) -> tuple[LaunchedJobID, ...]:
jobs that can be used to query or alter the status of that
particular execution of the job.
"""
return self._dispatch(dispatch.DEFAULT_DISPATCHER, *jobs)
run_id = datetime.datetime.now().replace(microsecond=0).isoformat()
"""Create the run id"""
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to put these comments ahead of the variable as non-docstrings, can we make these look like traditional comments?

Suggested change
"""Create the run id"""
# Create the run id

In theory it makes saves us a no-op instruction at runtime, but in practice I just think it looks nicer, hahaha

smartsim/launchable/job.py Outdated Show resolved Hide resolved
@amandarichardsonn amandarichardsonn merged commit 52cd8ec into CrayLabs:smartsim-refactor Aug 13, 2024
20 of 33 checks passed
@amandarichardsonn amandarichardsonn deleted the path-injection branch August 13, 2024 22:39
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.

3 participants