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

Application executes before colocated Orchestrator is created #522

Merged
merged 21 commits into from
Mar 21, 2024

Conversation

amandarichardsonn
Copy link
Contributor

@amandarichardsonn amandarichardsonn commented Mar 15, 2024

Orchestrator launch moved to a blocking process. Application executes once Orchestrator is built.

@amandarichardsonn amandarichardsonn added area: orchestrator Issues related to the Ochestrator API, launch, and runtime area: launcher Issues related to any of the launchers within SmartSim bug: major A major bug labels Mar 15, 2024
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.

A couple of comments on the first round but want to wait for @ashao and @al-rigazzi feedback

smartsim/_core/launcher/colocated.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/colocated.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.72%. Comparing base (d1dfac8) to head (636c81f).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #522      +/-   ##
===========================================
+ Coverage    90.67%   90.72%   +0.04%     
===========================================
  Files           60       60              
  Lines         3817     3816       -1     
===========================================
+ Hits          3461     3462       +1     
+ Misses         356      354       -2     
Files Coverage Δ
smartsim/_core/launcher/colocated.py 97.80% <100.00%> (-0.03%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Just one change requested, otherwise looks good!

smartsim/_core/entrypoints/colocated.py Outdated 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.

One small pedantic thing!

smartsim/_core/launcher/colocated.py Outdated Show resolved Hide resolved
ashao
ashao previously requested changes Mar 20, 2024
Copy link
Member

@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 tracking this down. Some minor suggestions requested.

doc/changelog.rst Outdated Show resolved Hide resolved
doc/changelog.rst Outdated Show resolved Hide resolved
smartsim/_core/entrypoints/colocated.py Show resolved Hide resolved
smartsim/_core/entrypoints/colocated.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

I suggest a small change to the filename, requesting @ashao's opinion on that too, to avoid too many iterations

smartsim/_core/entrypoints/colocated.py Outdated Show resolved Hide resolved
filename = (
f"{hostname}.log"
if os.getenv("SMARTSIM_LOG_LEVEL") == "debug"
else "/dev/null"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also suggest "/dev/null" -> os.devnull see here as a more portable (and nice looking) solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! would h8 for this to fail for others

Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

That was quick! LGTM

@amandarichardsonn amandarichardsonn dismissed stale reviews from ashao and mellis13 March 21, 2024 21:30

for merge

@amandarichardsonn amandarichardsonn merged commit 06d6166 into CrayLabs:develop Mar 21, 2024
44 checks passed
@amandarichardsonn amandarichardsonn deleted the colo_shell branch March 21, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: launcher Issues related to any of the launchers within SmartSim area: orchestrator Issues related to the Ochestrator API, launch, and runtime bug: major A major bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants