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

DATAUP-729: no more magic job attributes #2972

Open
wants to merge 3 commits into
base: DATAUP-729-just-job-ts
Choose a base branch
from

Conversation

ialarmedalien
Copy link
Collaborator

Description of PR purpose/changes

Removing the 'magic' from retry_ids and child_jobs so that they now return their current state instead of triggering a job look up.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-729

  • Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

  • Details for how to test the PR:
  • Tests pass locally and in GitHub Actions
  • Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • (Python) I have run Black and Flake8 on changed Python code manually or with a git precommit hook

…eturn their current state instead of triggering a job look up
@ialarmedalien ialarmedalien requested a review from n1mus May 13, 2022 18:55
@ialarmedalien ialarmedalien self-assigned this May 13, 2022
@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #2972 (609e5ab) into develop (63a8f02) will increase coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2972      +/-   ##
===========================================
+ Coverage    73.25%   73.27%   +0.01%     
===========================================
  Files           36       36              
  Lines         3903     3902       -1     
===========================================
  Hits          2859     2859              
+ Misses        1044     1043       -1     
Impacted Files Coverage Δ
src/biokbase/narrative/jobs/job.py 90.86% <66.66%> (+0.35%) ⬆️
src/biokbase/narrative/jobs/jobmanager.py 95.37% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 011eb19...609e5ab. Read the comment docs.

@sonarcloud
Copy link

sonarcloud bot commented May 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@n1mus n1mus left a comment

Choose a reason for hiding this comment

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

Should we add the new retry_ids in jm.retry_jobs? Either manually or with a force refresh

Copy link
Contributor

@n1mus n1mus left a comment

Choose a reason for hiding this comment

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

Oops going to change status to "request changes" because I forgot to ask about refreshing any new retry_ids

self.assertCountEqual(batch_job.children, reg_child_jobs)
self.assertCountEqual(batch_job._acc_state["child_jobs"], new_child_ids)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can go back in, but ofc like self.assertCountEqual(batch_job.child_jobs, new_child_ids)

@n1mus n1mus changed the base branch from develop to DATAUP-729-just-job-ts May 19, 2022 21:13
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