-
Notifications
You must be signed in to change notification settings - Fork 37
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
Command Generation #615
Command Generation #615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!! Some initial comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just some small changes!
d15e951
to
e9d8eca
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #615 +/- ##
====================================================
Coverage ? 32.93%
====================================================
Files ? 107
Lines ? 6401
Branches ? 0
====================================================
Hits ? 2108
Misses ? 4293
Partials ? 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work this is all so awesome!! Very well written, I have a couple suggestions below to just consider mayhaps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some initial comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more questions/comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! The CLI seems to be working wonderfully, I just have a few complaints about the original SmartSim V0.7 "to configure" operation that I think we should address as part of the this ticket.
Otherwise, LGTM!! Feel free to let me know what you think!
"5": 10, | ||
"FIRST": "SECOND", | ||
"17": 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly wrong, but thought I should call it out: in theory you should only need to handle a param mapping of type dict[str, str]
. If you can handle more types that's great but don't feel the need to explicitly test for it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is carry over from previous tests. I can remove it if you think its irrelevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say either assume a dict[str, str]
or introduce a type alias so that we know exactly what you are expecting before we serialize and send a command over the wire
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to re-look through the tests, but here is some quick comments regarding the configure operation refactor!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last super small thing!
def test_configure_invalid_tags(fileutils): | ||
"""Test configure operation with an invalid tag""" | ||
tagged_file = fileutils.get_test_conf_path( | ||
osp.join("generator_files", "easy", "marked", "invalidtag.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly hurt that you thought my test was easy, hahahaha
Co-authored-by: Matt Drozt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending tests!!
9b511f1
into
CrayLabs:smartsim-refactor
No description provided.