future: Rename 'state' from `"succeeded" to "finished"

[HenrikBengtsson]

Hi, I'm doing more cleanups on the future package and I noticed that you use "succeeded" to denote that a CivisFuture has finished successfully, e.g.

https://github.com/search?q=repo%3Acivisanalytics%2Fcivis-r%20succeeded&type=code

However, elsewhere in the future ecosystem, we use "finished" for this state. Could you please update your package accordingly? Looking at the above search, it could be that:

$ sed -i 's/"succeeded"/"finished"/g' R/*.R tests/testthat/*.R

is all that you need to do.

Updating this would help me remove a hard-coded special case for civis in the future package;

  ## FIXME: civis::CivisFuture uses 'succeeded' /HB 2019-06-18
  if (!future$state %in% c("finished", "failed", "succeeded")) {
    ...
  }

Thank you

[pcooman]

Hi @HenrikBengtsson,

Unfortunately, the "succeeded" state does not come from CivisFuture() itself, but from the Civis API. The possible states for a job in the Civis API are: "idle", "queued", "running", "succeeded", "failed", "cancelled", and "sleeping". To fully replace "succeeded" with "finished", we would not only need to replace it here in the civis-r repo (for the R Client), but also in the Civis API, and in the other Clients (for Python and JS).

Also, this change is highly likely to break user pipelines: a child job may be waiting for a "succeeded" response from a parent job before executing.

We would greatly appreciate it if you could keep the exception in place! Would that be possible? If not, there are a few things we could do:

1. Upper bound the version of the future package in the civis DESCRIPTION file. We currently Import future (>= 1.8.0), so we could add something like future (<= 1.31.0) to make sure our users only use the civis package with a future package that still has the Civis exception.
2. We could try to add a copy of the future::SignalConditions() function with the Civis exception into civis and redirect our future::Future() to use the civis::SignalConditions() function instead?

Do you have a preference? Or maybe another suggestion for how to resolve this issue?

[HenrikBengtsson]

Update

    future$state <- scripts_get_containers_runs(id = future$job$containerId,

so that the correct string in stored.

[HenrikBengtsson]

I've created futureverse/future#667 for improving on the documentation around the internal state object. I'll have to think more about exactly which possible values should be supported, but at least it'll help you go get the gist.

Unfortunately, the "succeeded" state does not come from CivisFuture() itself, but from the Civis API. The possible states for a job in the Civis API are: "idle", "queued", "running", "succeeded", "failed", "cancelled", and "sleeping". To fully replace "succeeded" with "finished",

To further clarify my previous comment. If you'd like to record the state returned by the Civis API, you are free to store those in your own custom field, say, civis_state. That will be ignored by the future framework. The state field is used by the Future class and has special purpose there. So, you must not use that for your own purposes.

Without knowing or having access to the Civis API, my best guess would be to map:

• Civis states "queued", "running" to future state "running"
• Civis state "succeeded" to future state "finished"
• Civis state "failed" to future state "failed"
• Civis state "cancelled" to future state "interrupted"

I don't know what Civis states "idle" and "sleeping" represent in the life cycle of a future object.

As I mention in futureverse/future#667, I might revisit the need for future states "failed" and "interrupted" later on, so right now, I'm mainly concerned about you "succeeded" mapping to "finished".

[HenrikBengtsson]

Was this comment retracted/deleted? I got an email notification, but I cannot see it online. /Henrik

…

On Mon, Feb 13, 2023 at 4:35 PM pcooman ***@***.***> wrote: I'm sorry, I'm not sure I understand. Are you suggesting I do something like this? resolved.CivisFuture <- function(future, ...){ if (!is.null(future$job$containerId)) { future$state <- scripts_get_containers_runs(id = future$job$containerId, run_id = future$job$id)$state } if (future$state == "succeeded") future$state <- "finished" future$state %in% c("finished", "failed", "cancelled") } Is there a way for the future to be recognized as resolved, but still have "succeeded" as its final state? I expect a lot of users have pipelines like: task_1 <- CivisFuture(...) if (task_1$state == "succeeded") { task_2 <- CivisFuture(...) } so setting the state to "finished" will cause these to break. Also, could you please clarify how the signalConditions () function will change? Is the plan to just take out "succeeded", changing this line <https://github.com/HenrikBengtsson/future/blob/d1218528b26871ba8d5bbf5d666d266adcf781e9/R/signalConditions.R#L30> to if (!future$state %in% c("finished", "failed")) { or to remove the whole block (lines 30-36)? Thank you! — Reply to this email directly, view it on GitHub <#245 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMKXUQRWZQIQPETMLJGS6DWXLHNBANCNFSM6AAAAAAT6ULL5M> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pcooman]

Yes, I deleted it after I saw your earlier comment.

[pcooman]

For now, I set an upper bound for the future package in our DESCRIPTION file (Imports: future <= 1.31.0)).

This is a temporary solution, as we are discussing internally on how to make this change without breaking user pipelines. Defining a new "civis_state" field would be a breaking change, that would require a major version update (to v4.0.0), so we are exploring possible alternative approaches. I don't want this to hold up your work on the future package for longer than it already has though.

[HenrikBengtsson]

For now, I set an upper bound for the future package in our DESCRIPTION file (Imports: future <= 1.31.0)).

I suspect this might cause more problems for users. I'm not sure, but I think it'll also break my revdep checks when I update future. I'm also not sure CRAN supports that - I'm not aware of any other packages that done that.

Defining a new "civis_state" field would be a breaking change, that would require a major version update (to v4.0.0), so we are exploring possible alternative approaches. I don't want this to hold up your work on the future package for longer than it already has though.

Thanks for moving forward. However, I honestly don't understand the problem. Standard encapsulation techniques should allow you to use whatever name is needed. If I can do:

  ## FIXME: civis::CivisFuture uses 'succeeded' /HB 2019-06-18
  if (!future$state %in% c("finished", "failed", "succeeded")) {
    ...
  }

in the future package, I don't see why you do similar thinkgs in the civis package instead.

Or, are you saying you have server code that rely on the field state and that it takes certain values?

[pcooman]

v3.1.0 was approved and is now available on CRAN (here). This version deprecates the "local" argument in CivisFuture and adds new API endpoints. It does still have the upper bound of future <= 1.31.0 in the Imports field of the DESCRIPTION, which is only meant as a stop-gap solution.

For a more permanent solution, I made (and merged) a new PR here to map the "succeeded" status we get from the Civis API to a "finished" future$state, in both resolved.CivisFuture (here) and result.CivisFuture (here). I was initially concerned about users depending on the state of the future in their pipeline logic, but that does not appear to be the case. Instead, once a future is resolved, users get the state of the API endpoint itself, using the await() function (which should still return a "succeeded" string). With this change, I also reverted the previous change in the DESCRIPTION file: Imports: future (>= 1.8.0). If we do encounter any unexpected issues, we will still have the new v3.1.0 to fall back on.

[pcooman]

@HenrikBengtsson, civis v3.1.1 is now available on CRAN (here). This versions removes the upper bound on future (Reverting back to requiring future >= 1.8.0, and maps the "succeeded" output from the Civis API to a "finished" future$state.

Please let me know if there is anything else that may be preventing you from making the desired changes on your end!

[HenrikBengtsson]

I ran it through my revdep checks, and it looks good now. Thanks.