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.