OK, this is a good example, actually feels "realistic"; I wish the others were more like this one!
The thing I didn't understand is that the subprogress can assign fractions of the total, even when we don't know what the total is. That gives a ton of flexibility, but also seems like it would admit logical errors? Like, if my totalCount is less than my total assigned count, the progressmanager will complete before its children are actually finished? And if it's more than my total assigned count, it will never complete?
It kinda feels, like Dave suggested above, that the API is wrong here — there should be two states, determinate & indeterminate, but totalCount in the determinate case should always be exactly the sum of assigned counts, it shouldn't be arbitrarily mutable.
And being arbitrarily mutable means that deterministic progress can go backwards, as I said in passing above. The API has constraints intended to prevent this, but doesn't really succeed in doing so.
To me, it suggests that the appropriate API here is one that splits the states of "mutable+indeterminate" (as in, can create and apportion children, but we don't know yet how much total work there is) and "immutable+deterministic" (as in, children can progress, but can no longer be added, and we know exactly how much total work there is).
Thanks for the update to the examples. I'll take a look but I'm also feeling extremely exhausted with this proposal and will probably be bowing out of reviewing it soon, regardless of its state. I feel like I've been making the same points over and over again and we're still ending up with something that feels very very similar to NSProgress with its over-wrought API and exists in Foundation instead of the Standard Library like it should.
Edit to spitball... As I've talked with others about this and we've ideated on things, we've wanted something that takes this kind of form:
let reporter = withProgressReporting { progressReporter in // also makes this @TaskLocal
await doLongRunningProgressThing(reporter: progressReporter)
}
let progressStream = reporter.observing(Property1.self, Property2.self)
for await (p1, p2) in progressStream {
// Property1 and/or Property2 has updated
}
or perhaps just:
let reporter = ProgressReporter()
Task { await doLongRunningProgressThing(reporter: reporter) } // no @TaskLocal progress
The ProgressReporter would need ways to add and remove sources of progress values as reportable tasks get added or cancelled or reconsidered. The Property1.self type might define how all the values get reduced together. Maybe there are default properties like isIndeterminate and isComplete.
Maybe that looks like:
func doLongRunningProgressThing(reporter: ProgressReporter) async {
let subThing1 = ProgressReporter()
try reporter.add(source: subThing1)
async let thing1 = doThing1(reporter: subThing1)
let subThing2 = withProgressReporting { doThing2() } // pulls the TaskLocal reporter
try reporter.add(source: subThing2)
reporter.add { await doThing3() } // creates implicit reporter in TaskLocal storage
}
And maybe reporting progress would tie off the transactional update stuff to Observables.
Or something.
Fundamentally though, it feels like a huge miss for this API to not be part of the Task system and for it to not be built in terms of AsyncSequence and Observable primitives.
let reporter = ProgressReporter()
Task { await doLongRunningProgressThing(reporter: reporter) } // no @TaskLocal progress
I think the fatal flaw in this idea is that it confuses who is responsible for what parts of the overall progress reporting.
Why is the reporter created with no arguments then passed to someone else? What state is mutable before and after it's passed along? Why do the two functions have to know about how each other are implemented in order to report progress?
Implicit progress reporting is also inherently broken. Example:
func doLongRunningProgressThing(reporter: ProgressReporter) async {
let subThing1 = ProgressReporter()
try reporter.add(source: subThing1)
async let thing1 = doThing1(reporter: subThing1)
let subThing2 = withProgressReporting {
innocuousFunction() // NEW
doThing2() // pulls the TaskLocal reporter
}
try reporter.add(source: subThing2)
reporter.add { await doThing3() } // creates implicit reporter in TaskLocal storage
}
Then later, someone in innocuousFunction, or something that innocuousFunction calls, which could be in yet another library, suddenly starts using the TaskLocal progress. Now all of the progress is consumed in innocuousFunction and doThing2() counts for nothing, even though subThing2s closure didn't change. This is the reason why we introduced the "discrete" progress reporting API on NSProgress many years ago, and why this proposal relies on argument passing to connect progress.
await withProgressTracking(totalCount: 10) { reporter in
await doLongRunningProgressThing(progress: reporter.subprogress(assigningCount: 10))
} observer: { progress in
print("Doing thing: \(progress.fractionCompleted.formatted(.percent))")
}
This uses structured concurrency to define the lifetime of the progress reporter, and the observer closure uses observation tracking to schedule itself to run again each time any observed property changes. The closure is called one last time after the primary closure finishes running to handle the “fully completed” state.
We don't need to use only structured concurrency to define the lifetime of the progress. We can just use regular structured code, which is what the consuming Subprogress is designed to do. As discussed earlier, when it goes out of scope, the progress is completed. Or, it can be consumed into a ProgressManager which assumes responsibility for completing the progress.
In any case, the request from the first proposal was that this feature support progress with multiple parents. In order to do that, we must break out of structured concurrency by providing the ProgressReporter type as something that can be used from anywhere, like in this example.
final class ScanAndProcessOperation : Sendable {
// Public progress reporting - not mutable, but observable
public let progressReporter: ProgressReporter
// ...
It seems likely that most progress observation is likely to be from passing one of these reporters into some kind of higher level view type like SwiftUI's ProgressView instead of observing it directly with a closure and calling print. However, if you want to do that, you can do it today since ProgressReporter is Observable. I think it is correct that this type builds on the settled discussions about how observation works and uses the stdlib primitive rather than attempting to build a new kind of observation again.
I think it's time for me to bow out of this thread. My final review is this:
-1
This API does not fit in the direction of Swift. It is being implemented in the wrong framework and suffers from incorrect naming and convoluted mental model. This API does not feel at home in Swift; it feels too complex for new users to approach, even more so than NSProgress. As this API currently stands, I don't see myself really using this in everyday work, despite the additions (which I appreciate) to attempt to accommodate more use-cases. I feel like, going forward, I will have more success sticking to NSProgress and twisting it to continue to fit my needs, or just making my own thing.
Maybe I'll continue iterating on my alternative ideas with others, but honestly I'm too burned out on this entire pitch to continue advocating the viewpoint that this is too complicated and we need to re-think the entire approach. So, I'm admitting defeat and throwing in the towel.
Thanks for the follow-up! I think it's fair to say that completely separating the mutability and indeterminism states from each other, and making sure that totalCount is immutable and no more children can be added once it is set, would more definitively ensure that progress will not go backwards. However, I think that imposes too much restrictions and takes away flexibility on how developers use ProgressManager.
To me I think it would make it clearer how to use the API — many of my questions over the several reviews would have been answered if that distinction was visible in the API, and if the totalCount property were computed rather than mutable.
I'm interested in what flexibility you think is lost by doing so, though.
(edit: I was composing this simultaneously with Charles' message above)
We're past the end of the stated review period, with only me & Dave responding, and both of us expressing exhaustion with retreading the same ground, over an API that doesn't feel Swifty, or type-safe, or fit for common use cases. An API that's being designed without any attempt to simulate real situations and use it and see if it works. An API that seems to inherit some of the worst of NSProgress (which never gained significant traction AFAICT), and makes it even more complex, without adding significant safety guarantees. And I agree with Dave, at this point, I think I'd be more inclined to roll my own or even use NSProgress, than attempt to adopt this.
I think you should also assume that the low response rate from people who were previously invested in reviewing this, is largely exhaustion rather than acceptance.
So I'll stand by my -1 at the top of the thread. And I'm also disinclined to participate in further review of incremental changes to this proposed API. I don't think that more small changes, or increasing the complexity a 3rd time, is going to magically save this. It's clearly a hard problem, but it feels like a hard problem that probably has an elegant solution. I think some time away to reevaluate the whole problem space and devise an innovative, Swifty solution that's easy to use correctly in all cases, will pay off.
During the two review periods, the author made several changes to address community and workgroup feedback:
Multi-parent support with an additional ProgressReporter type: While this would inevitably complicate the API design, further exploration proves that this is indeed going to be a useful addition that allows observing subtasks that aren't visible to the call sites. The API also handles edge cases to support this use case by detecting if there would be a cycle in the multi-parent use case.
The parent progress now has access to all the additional properties of its descendents even if they don't share the same additional properties. In the previous generic-based approach, supporting carrying additional properties was done via the ProgressProperties protocol. The parent was not able to read the statically declared additional properties unless it is also the same type. This limits the usability of observing subtasks via the root parent progress alone. This is doable now. The parent progress can also maintain its own state of the custom properties while aggregating those of the children.
Built-in synchronization to help coordinate mutating variables including totalCount: with the withProperties closure, it is now safer to mutate multiple values and read multiple values in one transaction. The totalCount variable is also changed to a read-only property for easy access.
Many great questions were raised during the review. One of the biggest was that progress reporting should be more closely integrated with Swift concurrency or included in the standard library. The Foundation Workgroup had evaluated this, but this would break the use case of multiple parents. In that case, we needed ProgressReporter to not be tied to any specific concurrency context. On the other hand, Subprogress does take advantage of the lifetime management provided by structured concurrency, and hence a recommended pattern.
The other questions raised were addressed by the author adding context and clarification to the proposal. The Foundation workgroup agrees with the resolutions the author provided:
Provided example code for converting from indeterminate to determinate progress.
Discussed whether this API should be included in the Swift standard library or the Foundation. This discussion was covered in the "Alternatives Considered" section.
Addressed the issue of progress not being readily representable as an integer fraction, such as a double from the existing C library. This was added to the "Future Directions" section.
Explained how the weight of each subprogress is not known until later and how to set the ProgressManager from an indeterminate state to a determinate state. This information was provided in the "Example Code" section.
Clarified that the ProgressManager or Subprogress will complete upon deinit.
Added Streaming Observation via AsyncSequence in "Alternatives Considered".
Expanded the "Alternatives Considered" section with other names suggested.
The Foundation Workgroup acknowledges that there is one open question about this proposal, that is a potential issue with missing updates in a system where the ProgressManager's values, such as fractionCompleted is updated concurrently. Since withObservationTracking synchronously signs up to receive notifications of changes, if an update occurs concurrently with the code reestablishing withObservationTracking on a different observation context, dropped progress may occur. The Foundation Workgroup acknowledges this potential issue and believes this is an issue that should be resolved in the implementation in Observation.
In conclusion, the Foundation Workgroup has decided to accept SF-0023 ProgressReporter. We appreciate all your suggestions on this proposal.