Skip to content

feat: Add an exec command to submit AP to a test workcell#193

Merged
scottyantipa merged 22 commits intomasterfrom
SA-wc-submit
Feb 16, 2021
Merged

feat: Add an exec command to submit AP to a test workcell#193
scottyantipa merged 22 commits intomasterfrom
SA-wc-submit

Conversation

@scottyantipa
Copy link
Copy Markdown
Contributor

No description provided.

GregoryEssertel
GregoryEssertel previously approved these changes Feb 6, 2021
Copy link
Copy Markdown
Contributor

@GregoryEssertel GregoryEssertel left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment thread transcriptic/cli.py Outdated
)
@click.option(
"--workcellId",
"-wc",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we use -w for submit.py. Do we want to be coherent?

Comment thread transcriptic/commands.py Outdated
try:
res_json = json.loads(res.content)
if res_json["success"]:
print("Success. View dashboard for scheduling job.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe putting the url with the correct port can be nice. as we have to add the :9000
On another note, can we redirect to 9000 automatically in the lab repo that would be great :D

Copy link
Copy Markdown
Contributor

@yangchoo yangchoo left a comment

Choose a reason for hiding this comment

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

Bunch of minor nits.

If you don't mind, could you add tests to test/cli_test.py and/or test/commands/.._test.py?

Trying to be good citizens about increasing test coverage here for new tests.

Comment thread transcriptic/cli.py Outdated
Comment thread transcriptic/cli.py Outdated
Comment thread transcriptic/cli.py Outdated
pass


@cli.command("exec")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we probably want to limit this by feature group. See @cli.command("submit", ...)

Comment thread transcriptic/commands.py Outdated
Comment thread transcriptic/cli.py Outdated
Comment thread transcriptic/commands.py Outdated
Copy link
Copy Markdown
Contributor

@yangchoo yangchoo left a comment

Choose a reason for hiding this comment

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

Almost there!

Thanks for adding the tests.

Please also add your changes to CHANGELOG.rst and kebab-case options

Comment thread transcriptic/cli.py Outdated
@scottyantipa scottyantipa requested a review from yangchoo February 9, 2021 17:58
@scottyantipa
Copy link
Copy Markdown
Contributor Author

@yangchoo ok this is finally ready

yangchoo
yangchoo previously approved these changes Feb 15, 2021
Copy link
Copy Markdown
Contributor

@yangchoo yangchoo left a comment

Choose a reason for hiding this comment

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

Thanks! Almost all done.

Pretty minor nit in terms of scoping exec under some feature flag.

Comment thread transcriptic/cli.py
commands.format(manifest)


@cli.command("exec")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: we probably want to limit this by feature group. See @cli.command("submit", ...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

discussed offline

@scottyantipa
Copy link
Copy Markdown
Contributor Author

Added workcell format validation

Copy link
Copy Markdown
Contributor

@yangchoo yangchoo left a comment

Choose a reason for hiding this comment

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

lgtm. When merging, please ensure we have a readable final commit message.

@scottyantipa scottyantipa merged commit d31cc1c into master Feb 16, 2021
@scottyantipa scottyantipa deleted the SA-wc-submit branch February 16, 2021 16:52
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