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

Workflow compute costs [VS-472] #7905

Merged
merged 27 commits into from
Jun 22, 2022
Merged

Conversation

mcovarr
Copy link
Collaborator

@mcovarr mcovarr commented Jun 16, 2022

Includes some of VS-441's driver WDL with the parts that are not workflow compute costs commented out. Successful run here: https://job-manager.dsde-prod.broadinstitute.org/jobs/2837665e-6903-4dc5-b9e1-418739d9c837

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@00f07e9). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             ah_var_store     #7905   +/-   ##
================================================
  Coverage                ?   86.291%           
  Complexity              ?     35192           
================================================
  Files                   ?      2170           
  Lines                   ?    164888           
  Branches                ?     17786           
================================================
  Hits                    ?    142283           
  Misses                  ?     16281           
  Partials                ?      6324           

@mcovarr mcovarr force-pushed the vs_472_workflow_compute_costs branch from 61a8945 to a99d2ae Compare June 21, 2022 13:09
@mcovarr
Copy link
Collaborator Author

mcovarr commented Jun 21, 2022

Added some sample output to the associated JIRA ticket.

@mcovarr mcovarr requested a review from gbggrant June 21, 2022 14:23
Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

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

looked over the sample JSON output and it LGTM 👍🏻

Copy link
Collaborator

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

Nice test coverage.


if workflow_name.startswith('Gvs'):
if len(workflow_ids) == 1:
# If this run is < 1 day old the cost data may not yet be available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it that the cost data might not be available - or is it that it might be not yet up-to-date? Perhaps put a general warning if the run is < 1 day old to tell the user that they might want to run this later just to be sure that cost is correct?

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 think this usually happens when this code runs too soon after the workflow completes.

I am capturing this message in a logfile but now that you bring this up I'm wondering if that's really a place that the users of this code would actually look; we should probably discuss as a team the best way to represent this condition so users can make an informed choice as to whether instances of this situation are okay or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

per mob discussion we're going to use this as is for now

@mcovarr mcovarr merged commit 8cd0f5f into ah_var_store Jun 22, 2022
@mcovarr mcovarr deleted the vs_472_workflow_compute_costs branch June 22, 2022 20:45
RoriCremer pushed a commit that referenced this pull request Jun 29, 2022
This was referenced Mar 17, 2023
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