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 stop after cycle point inconsistency with other cycle point options #4709

Closed
MetRonnie opened this issue Feb 22, 2022 · 10 comments · Fixed by #4827
Closed

Fix stop after cycle point inconsistency with other cycle point options #4709

MetRonnie opened this issue Feb 22, 2022 · 10 comments · Fixed by #4827
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@MetRonnie
Copy link
Member

MetRonnie commented Feb 22, 2022

(Re-written)

The final cycle point is only stored in the DB if specified via command line option --fcp, whereas the stop after cycle point is stored in the DB regardless of whether it was specified via command line option or flow.cylc.

This means changing the final cycle point in flow.cylc and reinstalling + restarting will use the new point, but doing the same for stop after cycle point will ignore the new point. Why should this be the case?

@MetRonnie MetRonnie added the bug? Not sure if this is a bug or not label Feb 22, 2022
@MetRonnie MetRonnie added this to the cylc-8.0rc2 milestone Feb 22, 2022
@MetRonnie MetRonnie added the question Flag this as a question for the next Cylc project meeting. label Feb 22, 2022
@MetRonnie MetRonnie changed the title Final cycle point not stored in DB? Should stop after cycle point defined in flow.cylc get stored in DB? Feb 22, 2022
@MetRonnie MetRonnie removed the bug? Not sure if this is a bug or not label Feb 22, 2022
@hjoliver
Copy link
Member

hjoliver commented Feb 22, 2022

So for "stop-after-point" the original DB value is used on restart regardless of attempts to override it?

It seems to me we should a) always store both in the DB; and b) always use a new value - if given (command line or flow.cylc) - on restart

The only complication I've thought of is that changing the FCP alters the length of the graph and brings forward any parts of it defined relative to the FCP. So ideally we'd be careful with that, but it might be difficult to figure out the consequences.

@MetRonnie
Copy link
Member Author

So for "stop-after-point" the original DB value is used on restart regardless of attempts to override it?

Yes unless you use --stopcp=<cycle-point> or --stopcp=reload, the latter will take the value from flow.cylc.

It seems to me we should a) always store both in the DB...

Store both? Why?

...and b) always use a new value - if given (command line or flow.cylc) - on restart

But the point of storing in the DB is to use the stored value on restart (unless overridden on CLI), so then what would be the point of storing in the DB at all?

@hjoliver
Copy link
Member

It seems to me we should a) always store both in the DB...

Store both? Why?

One defines the end of the graph, one says where to stop prior to that. Why would we not store both in the DB?

But the point of storing in the DB is to use the stored value on restart (unless overridden on CLI), so then what would be the point of storing in the DB at all?

Ah, I'm not sure if I've missed the point, or if I should have been more explicit!

At start-up, either item can be gotten from the config or from the CLI. At restart, the used value can be (attempted to be, at least) overridden on the CLI or via updating and reinstalling the config. Shouldn't we be keeping track of all of that, so we can figure it out even if the user changed the value in the config, after initially using the CLI?

@MetRonnie
Copy link
Member Author

One defines the end of the graph, one says where to stop prior to that. Why would we not store both in the DB?

Oh I see, you mean store both the stopcp and the fcp (I thought you meant store both the config and cli values of stopcp)

@MetRonnie MetRonnie added the bug? Not sure if this is a bug or not label Mar 3, 2022
@MetRonnie
Copy link
Member Author

MetRonnie commented Mar 3, 2022

Actually I think this is simply a bug introduced in a195573 (although this is part of the PR that added [scheduling]stop after cycle point in the first place)

The method Scheduler.process_cylc_stop_point() sets self.options.stopcp from the value of [scheduling]stop after cycle point if it is only defined there. Which means come

value = getattr(schd.options, key, None)
if value is not None and value != 'ignore':
self.db_inserts_map[self.TABLE_WORKFLOW_PARAMS].append({
"key": key, "value": value})

it gets put in the DB. However, the other cycle point settings in [scheduling] do not cause self.options.<whichever> to be set.

@wxtim
Copy link
Member

wxtim commented Mar 18, 2022

@MetRonnie - If you are happy to do so, please re-tag this as RC3.

@MetRonnie MetRonnie modified the milestones: cylc-8.0rc2, cylc-8.0rc3 Mar 18, 2022
@MetRonnie
Copy link
Member Author

MetRonnie commented Mar 18, 2022

Have discussed this with Dave, we think the best course is probably to make stop after cycle point behave the same as the other cycle point settings, which would fix #4637 (as an alternative to #4680), and then separately figure out (and document!) how we want all the cycle point settings/options to behave

@dpmatthews dpmatthews added bug Something is wrong :( and removed bug? Not sure if this is a bug or not question Flag this as a question for the next Cylc project meeting. labels Apr 4, 2022
@dpmatthews
Copy link
Contributor

This is no longer a question but I need to raise a follow-up issue before we close this one.

@wxtim
Copy link
Member

wxtim commented Apr 4, 2022

This is no longer a question but I need to raise a follow-up issue before we close this one.

Can I suggest that we change the name of the issue when it ceases to be a question?

@hjoliver
Copy link
Member

hjoliver commented Apr 4, 2022

This is no longer a question but I need to raise a follow-up issue before we close this one.

Can I suggest that we change the name of the issue when it ceases to be a question?

@MetRonnie can you change the issue title appropriately? (It's not entirely clear to me how the proposed solution relates to the original question, so not changing the title myself...)

@MetRonnie MetRonnie changed the title Should stop after cycle point defined in flow.cylc get stored in DB? Fix stop after cycle point inconsistency with other cycle point options Apr 5, 2022
@MetRonnie MetRonnie self-assigned this Apr 8, 2022
@hjoliver hjoliver modified the milestones: cylc-8.0rc3, cylc-8.0rc4 Apr 11, 2022
@MetRonnie MetRonnie linked a pull request Apr 20, 2022 that will close this issue
7 tasks
@MetRonnie MetRonnie modified the milestones: cylc-8.0rc4, cylc-8.0rc3 May 3, 2022
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 a pull request may close this issue.

4 participants