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

Ability to Upload Scheduling Goals to All Plans with Mission Model ID #70

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

zgoldberg22
Copy link
Collaborator

Addresses #69

Updated scheduling "upload" method such that if you do not have to specify the plan-id, the scheduling goals will upload to each plan in the specified model.

@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 16, 2023 16:05 — with GitHub Actions Inactive
plan_id: int = typer.Option(
..., help="Plan ID", prompt=True
help="Plan ID (optional)", default=-1, prompt=True #how to make this optional
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should the user still be prompted for the plan-id if it's now optional?

Right now, I still have it prompting the user for the plan-id, but if the user does not input anything and skips past it, then the default value is -1 (not a valid plan-id).

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to set prompt to False. IMO, the intuitive interface here would be to specify either the Plan ID or a Model ID, but not both, and throw an error if the user specifies both. If you want to preserve some sort of prompts, you can manually implement the logic with a menu (i.e., prompt the user to select whether they upload to a single plan or all plans for a given model). I have a utility that should make this easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like the Model ID is used within the the scheduling upload object, so I'm not sure if model id still needs to be an input when there is also a plan id. I could also query the model ID from the plan ID in order to assert that both are not inputted.
I will still make a selection menu so the user knows the two options and set both model id and plan id prompts to False.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point -- yeah, you should be able to get the model ID from the plan ID (using the same list_all_activity_plans method as below, if you want).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to not throw an error when both model id and plan id are inputted. I added a check if the model id is not inputted, then I find the model id from list_all_activity_plans.
I’m also making a couple checks to ensure the inputs that are needed are there using the select_from_list method. I set the schedule prompt to False to keep it consistent with the other inputs so they all inputs are prompted together.

@cartermak
Copy link
Member

cartermak commented Aug 16, 2023

Since there are already so many queries in this class, I might suggest you use an existing query instead of adding get_all_activity_plans_by_model. You can filter the results from list_all_activity_plans in the scheduling command, for example:

for plan in filter(lambda p: p.model_id == model_id, aerie_client.list_all_activity_plans()):
    plan_id = plan.id
    # etc.

@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@zgoldberg22 zgoldberg22 temporarily deployed to integration-test-workflow August 17, 2023 15:38 — with GitHub Actions Inactive
@cartermak cartermak had a problem deploying to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Failure
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak temporarily deployed to integration-test-workflow September 7, 2023 00:02 — with GitHub Actions Inactive
@cartermak cartermak added this to the v3 milestone Jan 29, 2024
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.

2 participants