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

Fix insert family submit-num increment #3236

Merged
merged 4 commits into from
Jul 28, 2019

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Jul 24, 2019

On insert of a family with tasks that had submitted jobs before, the submit
numbers of the tasks were not incremented. This changes fixes the issue.

These changes close #3210.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Includes an appropriate entry in the release change log CHANGES.md.
  • No documentation update required for this change.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label Jul 24, 2019
@matthewrmshin matthewrmshin added this to the cylc-8.0a1 milestone Jul 24, 2019
@matthewrmshin matthewrmshin self-assigned this Jul 24, 2019
@matthewrmshin matthewrmshin force-pushed the fix-insert-family-submit-num-incr branch from 320c365 to 117a0ec Compare July 24, 2019 15:42
@@ -134,7 +134,7 @@ def insert_tasks(self, items, stopcp, no_check=False):
continue
for taskdef in taskdefs:
task_items[(taskdef.name, point_str)] = taskdef
select_args.append((name_str, point_str))
select_args.append((taskdef.name, point_str))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One line bug fix here!

itask = self.add_to_runahead_pool(TaskProxy(
taskdef, point, stop_point=stop_point, submit_num=submit_num))
if itask:
LOG.info("[%s] -inserted", itask)
LOG.info("[%s] -submit-num=%02d, inserted", itask, submit_num)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better logging...

@matthewrmshin matthewrmshin force-pushed the fix-insert-family-submit-num-incr branch from 117a0ec to dd7210d Compare July 24, 2019 15:47
@kinow
Copy link
Member

kinow commented Jul 24, 2019

Travis failed (before the sensitive tests job).

@matthewrmshin
Copy link
Contributor Author

Travis failed (before the sensitive tests job).

And it is a real error! An unexpected consequence of the change. Looks like the database query will end up with over 1000 where clauses in this test. (It passes in my environment, but Travis CI's environment may be more restrictive?) I guess I'll have to change tactic.

@cylc cylc deleted a comment Jul 25, 2019
@kinow
Copy link
Member

kinow commented Jul 25, 2019

Code and test both look good to me, Travis happy too. +1

Only didn't approve because I kicked Travis three times already (one was a full restart), and it keeps refusing to pass this build 😠

@matthewrmshin
Copy link
Contributor Author

@kinow Thanks. I am taking a look at the failed Travis CI tests. The are passing in my desktop environment and I am sure they will pass on kicking. I'll move them to live with the flaky tests (or fix them if possible) in a separate PR.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Don't think Travis failure is related to this PR, so approving 👍 Thanks!

@matthewrmshin matthewrmshin force-pushed the fix-insert-family-submit-num-incr branch from f147703 to d7b50bd Compare July 26, 2019 11:09
@cylc cylc deleted a comment Jul 26, 2019
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Just waiting for TB prior to merge.

@matthewrmshin
Copy link
Contributor Author

(TB will likely fail in the same way as what I am trying to fix in #3244 . I believe there is a change to Travis CI on how it handles localhost.)

On insert of a family with tasks that had submitted jobs before, the submit
numbers of the tasks were not incremented. This changes fixes the issue.
Instead of building a single statement with multiple WHERE clauses,
execute statement multiple times to avoid issues when there are too many
WHERE clauses. This may be less efficient, but is much safer.
@matthewrmshin matthewrmshin force-pushed the fix-insert-family-submit-num-incr branch from d7b50bd to 18bd977 Compare July 27, 2019 21:13
@cylc cylc deleted a comment Jul 27, 2019
@hjoliver
Copy link
Member

(All checks passed!)

@hjoliver hjoliver merged commit c0187aa into cylc:master Jul 28, 2019
@matthewrmshin matthewrmshin deleted the fix-insert-family-submit-num-incr branch July 29, 2019 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

insert: job submission number not set when inserting family
4 participants