Hi all, thanks so much for your patience and continued engagement. I appreciate the time that all of you have put in to give thorough and thoughtful review, and I would like to address a couple of key points that have been brought up in both the prior pitch thread and this review thread:
- Tree vs Acyclic Graph Structure
While the current API draws a line by committing to a tree structure, this was a choice that we made while designing this API, grounded in two key observations:
- Most real-world progress reporting is naturally hierarchical, in which tasks can have subtasks, and progress reporitng needs to account for the progress of the subtasks within the progress of a task. Additionally, this tree structure also allows for nice integration with Structured Concurrency, which also has a tree structure. Developers can execute subtasks that report progress within a
withTaskGroup closure nicely.
- Allowing child nodes to report to multiple parents complicates the API surface and potentially performance issues by introducing additional issues with propagation: a single update having to notify multiple parents of changes, differing weights of child in its different parents, etc.
Other concerns raised with wanting an acyclic graph structure to model progress reporting after, seems to be due to the fact that the ProgressReporter, being initialized from a ~Copyable Subprogress instance passed into a method that reports progress, becomes locally scoped and inaccessible to other scopes from which developers may want to observe the changes.
For this particular case, I don't believe that an acyclic graph structure is necessary to address this concern. Developers can instantiate an intermediary ProgressReporter, which they would can hold a reference to, in order to receive updates from a subtask that reports progress with their locally-scoped ProgressReporter instantiated from Subprogress , as follows:
graph TD
Root --> Intermediary1
Root --> Intermediary2
Intermediary1 --> Child1
Intermediary2 --> Child2
I also understand that the dance to introduce an intermediary via reporter.subprogress(assigningCount: 1).reporter(totalCount: 1) is not the most ideal, so I am in the process of working out an improvement to do so.
- Reflecting State of Cancellation in
ProgressReporter
I agree that the interaction between structured concurrency and progress reporting needs to be addressed better in this proposal. In particular, we need to clarity the rules around what ProgressReporter will do when a task that it reports progress for is cancelled.
In short, the clarification here is that a ProgressReporter should always be completed. If a Subprogress is not consumed, it will be completed. If a ProgressReporter is deallocated before it is finished, in the event of a cancellation, it will be completed.
I don't believe that it's the responsibility of ProgressReporter to represent sub-cancellation state. The state of whether or not a task is cancelled up to the Swift concurrency system, and progress tree should be resilient, so ProgressReporter will always complete itself.
With this API, one direction I have in mind is also to more clearly define the boundaries between the progress reporting mechanism and the actual mechanism of doing work / executing tasks. A progress reporting mechanism reports the progress being made in an ongoing task, and does not assume any ownership over any task that it reports progress for, and at the same time remain resilient to changes in states of tasks. If developers would like to report the status of operation, such as what errors are being thrown, we don't think developers should count on ProgressReporter to represent that. With the ability to add custom properties to ProgressReporter, which is a new addition introduced in this API, developers are free to report custom metadata in a type-safe manner within a progress tree.
- Handling of custom properties in
ProgressReporter
The prior implementation doesn't support the dual mode of operation in each ProgressReporter, which is a flaw in the prior implementation. I've reimplemented this part, so that each ProgressReporter can support both incrementing its own custom properties, and providing developers the values of custom properties of the whole tree to be aggregated.
An example to illustrate this is as follows:
graph TD
Root --> Child
Child --> Grandchild1
Child --> Grandchild2
Assuming we have a custom property with name customCount of type MyCustomCount. If the Root itself has a customCount of 2, and Child did not specify customCount, whereas Grandchild1 and Grandchild2 each has a customCount of 5. If a developer asks for customCount at Root, via the withProperties closure, it will return 2. If a developer wishes to get the values of children from the rest of the tree, we added a method to get all values of type MyCustomCount, so that it will return [2, nil, 5, 5] in this case. We also add another method to help developers aggregate all values AdditiveArithmetic types in the tree, so then developers can get 12 from the provided [2, nil, 5, 5] when they wish for the values to be aggregated.
This is so that it is clear to developers what are the effects of writing code for getting and setting customCount within the withProperties closure, so that getting customCount of Root just means getting 2, and setting customCount of Root to 4 just means setting customCount to 4. Writing something like customCount += 2 when the customCount is 2 means that customCount will become 4. None of the aggregation will be done within here.
Developers would need to call a method to get all values of customCount in the tree, and another one to aggregate them, and that is so that we can support dual modes of operations on a single ProgressReporter.
In addition to the points above, I acknowledge that there are also several other points brought up, which will be considered as improvements in the future as part of the future directions for this API, some of which include:
- Developers having to keep track of how much
totalCount is available for creating a Subprogress, even in simple cases. Refactoring code may involve tracing the whole progress tree to ensure that all the counts assigned and completed for a root progress does not cause over-calculations. To address this concern, we may consider adding a macro to add total counts up for simple cases in the future.
- Progress is not always universally represented in
totalCount and completedCount, and can sometimes be represented as a Double. We may also consider adding a special kind of reporter which can receive double updates and propagate these updates up the tree in the future.
I will also update the proposal soon to contain the changes in the API and the discussions that we have had in the thread. Feel free to follow up in this thread if you have any clarifications, concerns and questions!
