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.

Sign up for GitHub

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

Jump to bottom

Apply typehints to smartsim._core.launcher.step.* #334

Merged
merged 28 commits into from
Aug 22, 2023
Merged

Apply typehints to smartsim._core.launcher.step.* #334

merged 28 commits into from
Aug 22, 2023

Conversation

ankona
Copy link
Contributor

@ankona ankona commented Aug 2, 2023

No description provided.

@ankona ankona force-pushed the oss338 branch 2 times, most recently from 76eb01b to 5543980 Compare August 2, 2023 16:22
@codecov
Copy link

codecov bot commented Aug 2, 2023 •
edited
Loading

Codecov Report

Merging #334 (f6fa2b5) into develop (c4bf40c) will decrease coverage by 0.33%.
The diff coverage is 34.61%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #334      +/-   ##
===========================================
- Coverage    87.36%   87.04%   -0.33%     
===========================================
  Files           59       59              
  Lines         3546     3551       +5     
===========================================
- Hits          3098     3091       -7     
- Misses         448      460      +12
Files Changed Coverage Δ
smartsim/_core/launcher/step/mpiStep.py 33.80% <10.52%> (-7.11%) ⬇️
smartsim/_core/launcher/step/localStep.py 88.57% <100.00%> (ø)
smartsim/_core/launcher/step/step.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@ankona ankona changed the title initial commit of applying mypy typehints to step classes Enable and apply typehinting to smartsim._core.launcher.step.* Aug 2, 2023
@ankona ankona changed the title Enable and apply typehinting to smartsim._core.launcher.step.* Apply typehints to smartsim._core.launcher.step.* Aug 2, 2023
@ankona ankona force-pushed the oss338 branch 5 times, most recently from 45df9d1 to f0be332 Compare August 3, 2023 15:36
@ankona ankona marked this pull request as ready for review August 4, 2023 00:42
@ankona ankona requested review from al-rigazzi and removed request for ashao August 7, 2023 18:05
@ankona ankona added the type: usability Issues related to ease of use label Aug 9, 2023
al-rigazzi
al-rigazzi requested changes Aug 16, 2023
View reviewed 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.

Good stuff! I like the convenience functions and the improved readability of the code. I have left a couple of requests and some comments that I'd like to be addressed before merging.

smartsim/_core/launcher/step/alpsStep.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/alpsStep.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/alpsStep.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/localStep.py Outdated Show resolved Hide resolved
smartsim/_core/launcher/step/lsfStep.py Show resolved Hide resolved
smartsim/_core/launcher/step/pbsStep.py Outdated Show resolved Hide resolved
al-rigazzi
al-rigazzi approved these changes Aug 21, 2023
View reviewed 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.

LGTM! Thanks for getting this in and addressing my concerns!

@ankona
ignore test on systems without tensorflow
fe777fa
@ankona
remove unused import
fe742dc
@ankona
exe_args protected access end-run
e384ab9
@ankona
disable mypy
d29a6e3
@ankona
linter
727d921
@ankona
raise exceptions when run_command is missing
9f4eed4
@ankona
bomb on missing run commands...
ead4c51 
by assuming the --wdir implies run cmd cannot be optional
@ankona
minor refactor of batch/run/mpmd access for typing
dec0aca
@ankona
whitespace
cedadca
@ankona
whitespace
608b40b
@ankona
minor tweak to avoid obtuse nesting
bf975eb
@ankona
create _get_exe_args_list to avoid disable=protected-access
1f15db2
@ankona
mitigate Any-ness of results when using hasattr
4da332b
@ankona
whitespace for linter
f34fcdf
@ankona
Add docstring to run_settings property on Step subclasses
ca2aac6
@ankona
Docstring format fix
dcb37aa
@ankona
Rename parameter for clarity, add docstrings
57681a2
@ankona
Include detailed type info in error
004acb7
@ankona
Remove extraneous properties used to cast base types
2b54eab
@ankona
lint fix
e835af3
@ankona
Fix failure to check alloc vs run_settings
c6982c2
@ankona
Remove unnecessary passthrough properties
f6fa2b5
@ankona ankona merged commit 154f19c into CrayLabs:develop Aug 22, 2023
10 checks passed
@ankona ankona deleted the oss338 branch April 4, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al-rigazzi al-rigazzi al-rigazzi approved these changes

@MattToast MattToast Awaiting requested review from MattToast

@mellis13 mellis13 Awaiting requested review from mellis13

Assignees
No one assigned
Labels
type: usability Issues related to ease of use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankona @al-rigazzi