Add removed_intervals to /state
#182
Conversation
| @@ -230,6 +230,10 @@ absolute timestamps. | |||
| containing human-readable time durations, given in a slight modification of | |||
| the [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) extended time format: | |||
| `(-)?(h)*h:mm:ss(.uuu)?` | |||
| - Time intervals (type **`INTERVAL`** in the specification) are strings | |||
There was a problem hiding this comment.
What if the end of the interval is not known yet.
I think that is a case @johnbrvc and @clevengr would like to have, where they 'pause' the contest and so only know the start of the interval and later update it. Or are you saying in that case they should only emit a state update event once the end is also known?
There was a problem hiding this comment.
Agreed, one of the use cases is something like a fire alarm where you literally hit the 'stop' button and walk out the door. Yes, we could leave the clock running in subsystems and set the end/update the event afterward, but it would be nice for end time to be optional.
There was a problem hiding this comment.
The spec does not allow for open intervals. A removed interval always has a beginning and an end.
In the case you describe, i.e. you know when you want the removed interval to start but not yet when it will end, there are two possible solutions. You could either not set any removed interval yet, or you could set it to some value "far enough" in the future. Either is fine.
There was a problem hiding this comment.
Do we want the spec to allow for open intervals? I'm not in favor or against, but if we are thinking about implementing removed intervals properly now, this is the time to discuss this I'd say.
There was a problem hiding this comment.
I see Fredrik has already 👎🏼 our comments and maybe we don't. 🙂 I'm just not sure I love forcing one which could end up with either nothing sent ('contest is still running' when clearly it isn't) or far future ('contest will resume in 1865 days') is great either.
Instead of adding a new type (will we need INTERVAL anywhere else?) why not a child object like location: { start: TIME, end: TIME }?
There was a problem hiding this comment.
Do we want the spec to allow for open intervals? I'm not in favor or against, but if we are thinking about implementing removed intervals properly now, this is the time to discuss this I'd say.
I don't think we do. The simplest argument is that there is always a trivial workaround. Just pick any large enough end and you practically have an open interval anyway.
I'm just not sure I love forcing one which could end up with either nothing sent ('contest is still running' when clearly it isn't)
Well, it is though. If it actually isn't running then it has ended, and there are other messages you should be sending.
or far future ('contest will resume in 1865 days') is great either.
Yeah, I agree that this is a bit ugly, but it's or the most cornery of cases, I think it's fine.
Instead of adding a new type (will we need INTERVAL anywhere else?) why not a child object like location:
{ start: TIME, end: TIME }?
That's a great point, and in fact that is exactly how I first wrote it. That said, there is a standard ISO format for time intervals in ISO 8601, and we use ISO 8601 formats whenever available elsewhere, so it makes sense to do the same here. I think as I wrote it now is cleaner and better, but I don't feel very strongly about that.
There was a problem hiding this comment.
I'm just not sure I love forcing one which could end up with either nothing sent ('contest is still running' when clearly it isn't)
Well, it is though. If it actually isn't running then it has ended, and there are other messages you should be sending.
I meant more that if all the CCS has is a pause and resume capability, they might decide not to send an event on pause, which would be missing the point of this change. We should set the expectation that it should send an event on pause with a long interval, and make sure someone from PC^2 has reviewed this too b/c I think that's what they have.
Instead of adding a new type (will we need INTERVAL anywhere else?) why not a child object like location:
{ start: TIME, end: TIME }?That's a great point, and in fact that is exactly how I first wrote it. That said, there is a standard ISO format for time intervals in ISO 8601, and we use ISO 8601 formats whenever available elsewhere, so it makes sense to do the same here. I think as I wrote it now is cleaner and better, but I don't feel very strongly about that.
I lean slightly the other way that it's not worth adding a global type for this when time would be enough - but I don't feel very strongly about it either. :)
There was a problem hiding this comment.
I meant more that if all the CCS has is a pause and resume capability, they might decide not to send an event on pause, which would be missing the point of this change.
Sure, but they also could send an event.
We should set the expectation that it should send an event on pause with a long interval,
Do you mean adding a comment to the effect of "If the end of the removed interval is not known, a system should provide a best estimate, or at least some very large (such as 1 day later) value for the end"?
There was a problem hiding this comment.
[Somehow it seemed I added a duplicate comment yesterday, then removed one, and both were gone. Hree goes again.]
I think I'm not too much a fan of open-ended removed intervals, but if we support that, I object to using an end time far in the future to indicate that. I think it's misleading and might lead to problems, for example, if that end time is not that far in the future and it passes without someone noticing. Also, it means that this end time will get modified again, and I think we should try to write the specification to make things as immutable as possible.
Also, in general, I feel like it would be better to just use separate ISO date/time fields for start and end time, because a (well-defined) interval can have various formats:
<start>/<end>
<start>/<duration>
<duration>/<end>
I don't see the benefit of allowing these.
| | thawed | TIME ? | Time when the scoreboard was thawed (that is, unfrozen again), or `null` if the scoreboard has not been thawed. Required iff `scoreboard_freeze_duration` is present in the [contest](#contests) endpoint. Must not be set if frozen is `null`. | ||
| | finalized | TIME ? | Time when the results were finalized, or `null` if results have not been finalized. Must not be set if ended is `null`. | ||
| | end\_of\_updates | TIME ? | Time after last update to the contest occurred, or `null` if more updates are still to come. Setting this to non-`null` must be the very last change in the contest. | ||
| | removed\_intervals | array of INTERVAL ? | Array of [removed time intervals](ccs_system_requirements#removing-time-intervals). |
There was a problem hiding this comment.
We should add a requirement that intervals are strictly non-overlapping (also not "touching", since then they could be merged) and I'd say we should require the list to be sorted.
There was a problem hiding this comment.
- Definitely agree with non-overlapping.
- Not sure about "touching". On the one hand, what could the use possibly be to allow them? OTOH, it feels like overspecifying a bit.No every dumb thing should be explicitly forbidden.
- Not sure about sorted, but I guess that's nice.
I'm certainly not against any of those improvements. Do you want to push a fix, or should I?
Contest_API.md
Outdated
| thawed either, or, it may be finalized before it is thawed. That, is the | ||
| following sequence of inequalities must hold: | ||
| in any order. `removed_intervals` is not a state change, and so is not affected | ||
| by this requirement. For example, the contest may never be frozen and hence not |
There was a problem hiding this comment.
This added sentences makes the flow of the example afterwards a bit worse, but I don't directly see a good way to fix that.
There was a problem hiding this comment.
Yes, I very much agree. I also did not see a good way :(.
|
On Wed, Dec 11, 2024 at 5:26 PM Jaap Eldering ***@***.***> wrote:
***@***.**** commented on this pull request
In Contest_API.md
<#182 (comment)>:
> @@ -230,6 +230,10 @@ absolute timestamps.
containing human-readable time durations, given in a slight modification of
the [ISO 8601](https://en.wikipedia.org/wiki/ISO_8601) extended time format:
`(-)?(h)*h:mm:ss(.uuu)?`
+- Time intervals (type **`INTERVAL`** in the specification) are strings
I don't like having a removed interval with an end time far in the future
just to "represent" the fact that it is unknown. That means that the
interval needs to be updated again when the end time becomes known, and
that can lead to extra inconsistencies.
But it would have to be updated again when the time becomes known even if
it's set to "unknown" as well, so that's no different.
What if the "far in the future" end time actually passes without anyone
paying attention?
If "far in the future" is well chosen, that can't happen. (And I did
specify far **enough** in the future, which, by definition, is enough).
I'm not really a fan of supporting open ended intervals, but I strongly
feel that if we do, we should do so properly by having separate start and
end fields and that the end can be null.
Agreed, two separate TIME fields would be better than an INTERVAL, TIME
"union".
Furthermore, ISO 8601 supports three different alternative formats for
well-defined intervals (not just durations):
<start>/<end>
<start>/<duration>
<duration>/<end>
But I did specify that we would only accept the "start and end" variant.
I don't really see the benefit of allowing all that over simply specifying
a start and end date/time in normal ISO 8601 format.
—
… Reply to this email directly, view it on GitHub
<#182 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABSFKN6LQDOYO6FFJ5MQML2FDCZTAVCNFSM6AAAAABTLPD5O2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOJXGIZTGMJZGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I see @niemela replied to my deleted reply, see #182 (comment).
Sounds good!
Sorry, I missed that.
|
|
Here is my $0.02. The most common use case (and this has happened two times (to my recollection) at major events: once at WF and once at NAC and it happens MANY times at regionals) is simply pausing the contest for some indeterminate amount of time. I know programmers/engineers hate words like “indeterminate”, but they are a fact of life, so we have to deal with them in a non-hacky way. Making “indeterminate” == “some arbitrary future time” is a hack to make programmer’s lives easier and, IMHO, is not a good solution. I think there has to be a concept of a “paused” state in the contest (where contest clock is stopped - Note: this does NOT mean the contest has ended). This could be implemented as a “paused” property in the state message indicating the time the contest was paused, null if the contest is not paused. Downstream clients can then choose what to do if this property indicates a paused contest. In addition, an array of completed time intervals could also appear in the state message, providing a history of all time intervals the contest was paused. A completed time interval object (INTERVAL?) containing some representation of a time interval and the contest clock at the start of the interval. It seems there must be a concept of the contest clock being paused. During this time, no submissions are accepted by the CCS. A CCS will have to provide this information to teams on whatever GUI is used to make submissions. The “contest_time” property is provided in all relevant messages. The contest_time is what should be used for penalty calculation. The time property provides a record of when (wall time) this event actually happened, and shouldn’t be used for penalty calculation. I am unclear about how the proposed solution using just deleted time intervals will work from a CCS (or even a CDS) standpoint. Consider a completed contest (finalized) that has 2 of these so-called deleted time intervals. What exactly does the event-feed endpoint provide for events that happen during, or after a deleted time interval? Is it:
I think clear business logic has to be written up for what happens on endpoints and CCS team (and judge) GUI displays. I have not seen any discussion about this yet. Ex. A CCS should always show the contest time an event occurred, taking into account any so-called deleted time intervals. The CCS may optionally show the actual wall-time an event occurred. (Events in this case are submissions and clarifications and possibly judgments). As some folks pointed out, PC2 does this, as it maintains BOTH the contest_time and time of each event already. In the case of a contest pause, the contest clock is stopped. The contest clock is resumed when the contest is resumed (completing a so-called deleted time interval). In summary, I guess my suggestion would be to have the state message have a paused property and an array of COMPLETED(!) INTERVALs as mentioned in the previous discussions (but INTERVAL should include the contest_time at the start of the interval). If the paused property is non-null, it contains a TIME indicating when the contest was paused. This property remains set to that TIME until the contest is resumed, at which point a new INTERVAL is added to the array of INTERVAL. A CCS must set the paused property if the contest is paused, and then update the array of INTERVAL with a completed interval when the pause ends. However, incomplete or “made-up” INTERVAL’s should not be added to the array of INTERVAL until they are really complete. |
One "point of order". The purpose of this PR is to add information about the "removal of time intervals" that already exists in the spec to the API. The spec does not support the concept of a paused state. So, I suggest moving that to a separate ticket and discussion. I would also quite strongly oppose adding such a concept.
I'm a little bit confused by this comment. An interval as currently defined in the PR consists of the start and end ("wall time")of the interval. Why do you want the start and end wall time and the start contest time? The start contest time would be completely defined by other data already available, so it would be duplicate data.
Could you give some concrete examples of questions that are unclear? "GUI displays" are a little bit outside of the scope, but the main thing that is affected by removed intervals is the current contest time, and that would often be displayed, so I guess that is what you are referring to? What is unclear about that?
This is definitely outside the scope of the API spec. But I think CCS requirements says exactly what you suggest (i.e. a CCS must show the contest time, but is not required to show the actual wall-time.
It's not clear what you mean by "made-up", but I hope we would allow timestamps in the future. E.g. when it has been decided that the contest will start (and therefore the removed interval will end) and some specific time in the future. The "until they are really complete" seems to imply that you don't want to allow timestamps from the future? |
|
A fair question remains what happens in the event feed if at wall time X I add a removed interval going from X-A to X-B, so in the past. The wall time of the events in the event feed won't change, but this WILL have an impact on the contest time of any events after X-A. Do we send updates for these events? This seems the most logical thing to do, but we need to make sure the state event appears in the event feed at the correct time, even after replaying the contest. It has to appear in the feed basically just before these updates are send, right? Otherwise the event feed is in a quite inconsistent state (no pun intended). |
Correct.
Yes.
I don't quite see why that is so important? Isn't the event feed in an equally inconsistent state in the middle of the change regardless of whether we send the removed interval event first or the updated other events first? And this us the kind of "eventually consistent" semantics that we have elsewhere. |
Hmmm maybe you are right. Removing intervals in the fast is just super annoying. As soon as you do that, the scoreboard needs to be fully recalculated. But yeah, eventually it should be fine. |
|
Maybe we could say something like "implied events are optional". I.e. adding a removed interval implies that a bunch of other events gets updated contest time, meaning they must be resent, so sending them would be optional. (Because a client can either calculate those changes themselves, or if they don't want to do that, visit the actual endpoint that must return the updated information. It's a bit of a wild idea, and it is (also) outside the scope of this PR... |
I kinda like it and hate it at the same time. The feed is 'lying' if we don't send them, but we save a lot of events that indeed can be calculated. |
Yup, me too. It's so clearly "wrong", but it's also obviously "better"? I guess it's pragmatic? |
|
Regarding Fredrik's "point of order" (that this PR is about Regarding @johnbrvc 's somewhat detailed response, I generally agree with what he has said. My own $.02 is the following:
|
I thought I was adding information: a proposed new state property to fill out a deficiency (IMHO) in the current removed time interval definition. That is what I was doing. Since, IMHO, "removal of time intervals" is insufficient for what these time intervals really are, which is: times during which a contest was paused or IS paused. What else could a removed time interval be?
I can think of at least one case where this may not be not true. In any event, since the contest_time is not readily available, I don't think it can be considered "duplicate". It can be derived from other messages (contest information, along with the other state INTERVALS).
First, I don't think the CCS Requirements Doc clearly states what the API business logic is. The requirements doc does not say when the intervals are actually removed... During the contest, or after it, and, if the API endpoints are affected. I thought I did propose a concrete example of what I was asking: what does the event feed use for contest_time during, and after intervals? As a corollary, what do API endpoints return during and after removed intervals?
I was merely suggesting that this information has to be available in some way to a downstream client (like a CCS) - which, apparently it is as long as the time isn't munged.
How can a deleted/removed interval be added if it is not complete yet? To me, that makes no sense. You can't delete an interval in a contest if the interval hasn't finished yet. That was my reason for proposing the "paused" property. As a result, I suppose I am saying that a deleted/removed interval should not have a time in the future. In this case, the "paused" property would contain the start time of the pause. When the pause is over (contest resumed) then, and only then, could a new INTERVAL be added. That is what I was proposing. It seems a kludgy to have a deleted/removed time interval with a time in the future. |
But, the spec does in fact not support "pausing". I will agree that removing intervals in the way specified is very close to pausing, but it is not the same thing as a "pause the contest now" button, which is not required by the spec.
Agreed, If we decide to support pause (which I very much oppose), then @johnbrvc's suggestion seems like the correct way to do it.
What is a paused interval? I assume you mean "removed interval"? |
Oh, you absolutely were. I'm trying to separate different questions into different discussions though. To @clevengr's point though, these "different" discussions might be so intertwined that they can't be separated? I don't think so, but I can see the point. My point of view, would be be that this discussion should be about how to encode the current definition of "removed intervals", that we all know and hate, into the API, which should be relatively easy. I mean pretty much the only real question is should we use ISO 8601 time intervals or our own custom structures with ISO 8601 timestamps. (And the options aren't really that different either). The discussion of introducing a new concept to the spec of "pausing" is a bigger discussion and IMO, should be separated.
But that is NOT what it IS. I understand that you might want to change that, and that's absolutely fair. But the current definition of removed intervals is not the same as pausing, and that was on purpose. We did have pretty much this same discussion some 10 (?) years ago.
Please share this case?
Yes, that's a very fair point. It is not trivial to derive it.
I don't understand this point. The intervals are removed exactly when they are removed. I.e. if you remove them after the contest (which is allowed), then they are removed after the contest, and if they are removed during, then they are removed during. It can be either.
Yes, you did. Was that all the cases? I think I answered that one? Contest times gets updated at the instant a time interval is removed. API endpoints should return contest time as defined by the start time, wall clock time, and all currently defined removed time intervals... at all times.
Correct.
In the exact same way as you can have a (scheduled) start time before it has happened. You can schedule a removed interval to end at some time in the future. (@clevengr has mentioned in the past and in this thread that some contests "pause" for lunch break, if so you could certainly schedule both the start and end time, and thus both of them would be in the future).
But, you clearly can. If you put timestamps that are in the future, then you have. I think you mean that you shouldn't?
Why would we not allow the restart to be scheduled? We do allow the start to be scheduled after all?
|
But the spec does state the following (at What is a "disregarded time interval" if it is not a "pause" (I can see no distinction between the two). But, perhaps you are arguing that this is not quite the same thing as requiring a "pause the contest now" button? I guess I would agree -- but it is still a clear requirement that there must be a way to specify a removed time interval. I would argue that specifying such an interval is precisely the same thing as saying "the contest was paused during this interval". And, given this, I would argue that downstream clients (whether they are hitting specific endpoints or are listening to the event feed) should be made aware that such a thing (a change in the contest state) has happened. ...
Yes... since in my view a "removed interval" is for all practical purposes an "interval during which the contest was paused". |
Exactly.
👍
Agreed.
Agreed, if there is any difference it's only word choice, but the meaning is identical.
Agreed. (And this is in fact what this PR is about) Note though that the spec does not allow for removing open intervals, only intervals with a start and an end. Half-open intervals with a start but not an end, is what I would call "pausing", and that is not currently allowed. |
|
After thinking a bit I do think we should indeed split pausing and removing intervals. The use case for food breaks, etc. can be fully handled with removing intervals, without pausing the contest. I would even argue it's a better approach, since you can already announce (systematically, using the API) that the food break will happen, before it happens. If we would only have pausing of the contest (which no one is actually arguing for, I think), that would not be possible. I'd argue that removing intervals is also sufficient for the food breaks, since I think you don't need any (of course the bomb threat thing is a separate discussion which might need that notion) |
|
Sigh, that was a lot of reading (and sorry, had to skim...) for one weekend. Apologies if I missed responses to either of these, but as a downstream client/proxy two things stood out:
In state today we don't have a 'running' property, we have a 'started' and 'ended' time, i.e. we mark events and clients use them to decide what state we're in. So to me a paused property would be both inconsistent and unnecessary, because 'currently paused' = there's an interval that starts before now and either doesn't have an end or end is in the future.
For the same reason as above, I don't see the need for this. Clients don't care why the contest clock isn't moving for a period of time (and if they do, we could have |
I think one of the "sticking" points here is removed intervals that don't have an ending time. (Open ended intervals). It was proposed to use a "made-up" time in the future for such things, but this would be a kludge at best. |
The system time changes at some point during the contest (either by sysadmin or NTP or something similar). This will not affect the contest_time but will affect time, and consequently, any calculations that are based off of time. |
I can't speak for the other DOMjudge developers, but I don't think we will handle this case in any near future. |
I kinda like the idea of an (optional) |
Yeah, ouch, I think my answer to that is "please don't ever do that". And if it does happen, I would argue that the correct way to handle it within the spec is to resend every event with a |
|
As a note, we probably shouldn't implement it right now but can consider it in the future, and maybe it should affect the API shape (like field names, and in which objects to put things). The CSS for Northern Eurasia Finals contests has the concept of "clock." Basically, "clock" is a mapping between wall time and contest time. Each team has a clock assigned to them. Contests normally have a single clock, and than it's not much distinguishable from the state object after this proposal. There are some reasons why you might have several clocks in the same contest:
Possibly, the more generic way of expressing what you are trying to do here would be something like:
The nice thing about it, as it also covers countdowns with stops, even with the history, how it was changed (as you just need to specify wall time -> negative contest time mapping). Not sure if there are any use-cases for such information though. I think we can leave it as suggested here for now, but just keep in mind this generalization in case we decide we want to support pausing a single team, or support virtual contests in spec. |
eldering
force-pushed
the
removed-intervals
branch
from
1ae78d1 to
25b8f8b
Compare
|
As decided on the call, this will be be closed and a new PR will be written... |
Also adds a new type
INTERVAL, as one of the ISO 8601 time interval formats.