This new version includes the following changes in response to Foundation Workgroup's comments:
Multiparent Support: With the strong interest in multiparent progress shown in the pitch and review threads, the author should explore the possibility of incorporating such an API for ProgressReporter.
Streaming Progress Observation via AsyncSequence*: The author should evaluate the feasibility of implementing AyncSequence-based observation.
Clarification of Rules Regarding Cancellation: The API author should provide clarity on the expected behavior when the task being observed is canceled, as the new ProgressReporter does not support explicit cancellation.
Reviews are an important part of the Swift-Foundation evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager by email or DM. When contacting the review manager directly, please include proposal name in the subject line.
Once again, we seem to be missing the repository of worked examples that actually show how to use this. This API is so complicated that I don't know how to accurately evaluate it without working code I can run and tinker with. And I don't think it should be "on me" to extract the pull request into something I can build stand-alone and write those examples myself.
This iteration of the proposal seems to remove automatic reduction of custom properties in favor of a method on ProgressManager that gets all the current values from the subtree. I don't see how to turn this into an aggregate — eg. if I recursively search the filesystem and make a Subprogress per directory and per file, arbitrarily nested, and each file reports a byte count during progress, how do I sum those counts to display in my UI? How do I even know that the childrens' values have changed? Observation doesn't seem to fit this bill, and even if it did, it seems like it would be incredibly difficult to use in this way. ProgressReporter doesn't surface them either, so I can't do the reduction at the top level in my UI code either. It really feels like if this is to support custom properties, it needs to handle the reduction internally.
Oddly, the function that returns the values from children returns an array of optionals. I guess in one sense it makes sense to report nil from children which don't have the value, but I don't see how that information is useful. And its presence might encourage consumers to "know the structure of" their subtree and assign significance to particular values based on their index, which I feel would be undesirable. I think (if this is even the right API, see above) it should return a compactMap'd version which makes it clear you can't know which child a given value comes from. Alternatively, children should be identified, and it should return a map of them.
My previous feedback about "integer counts, divided in advance" being unsuitable for indeterminate progress remains unaddressed (or if it is addressed, there is no example of how it is expected to work).
I think the new three-type structure is hard to understand, and particularly the difference between ProgressManager and ProgressReporter is vague. I think the difference is that ProgressManager can have only 1 parent, where ProgressReporter can have many, but since ProgressReporter seems to have a strict subset of the functionality of ProgressManager I don't understand the need for the additional type.
Overall, I'm tired. By now I've put an enormous amount of effort into trying to understand the various iterations of this proposal, their flaws, their suitability for the purposes to which I might put them. And here on revision 5 after three threads of extensive feedback, I don't think I should need to be repeating the same questions I've asked at each iteration; the answers should be spelled out clearly in the proposal.
tl;dr: -1 from me again. It needs to be clear how to use this for common cases such as indeterminate progress, and aggregating progress values other than completion percentage. Ideally it should be self-explanatory from the API, how to do those things. Certainly, after reading the proposal, I should know how to do those things.
I've been thinking about the use of Observable here, and I don't think it can actually achieve what it's supposed to — it's intended for example that it could be an @State in a SwiftUI View, or that a CLI tool could iterate an async sequence of it using SE-0475. But both those use-cases require that the observer cannot miss a value from the ProgressReporter.
However, the only tool they have to observe values is withObservationTracking, which synchronously signs up to receive a notification of the next change. This is fine when mutations occur in the same isolation; you can guarantee that no mutations can occur between being notified of one mutation & signing up for the next. There's a lengthy discussion of this point in the SE-0475 review thread. However, these progress types are Sendable, and their properties may be mutated from an arbitrary isolation domain, including concurrently with the code reestablishing the withObservationTracking. I believe this guarantees that even an async sequence observing one of these types will miss progress updates. And SwiftUI is in an even worse position — it's not in any hurry to reestablish withObservationTracking for the Observable types it cares about; it "knows" everything is synchronous on the main actor. So it potentially waits nearly a whole frame (during which it will miss asynchronous updates) before reestablishing observation.
I actually think it's "unsafe" (in the sense that no observer can guarantee even eventual consistency — it's always possible that they miss the last-ever update) to have a Sendable & Observable type. But I'd like to summon @Philippe_Hausler to tell me I'm wrong...
The observer does not vend the "eventual consistency" but instead it is the type storing the data in conjunction with the observation itself. The window of opportunity of a missed value only means that progress might expect 47/100, 48/100, 49/100... but instead get 47/100, 49/100... but it should always get the finished case (since that is a persistent state transition to finished).
The onus here is upon progress itself to vend the events in a thread safe accessable manner; which progress does iiuc from the pitch.
As a defensive measure I would recommend that any async sequence observation of progress claim that the end of sequence (without throwing) infers completion of the progress. Here is a do/don't for what I'm suggesting (note this is a purely defensive coding approach).
/*defensive*/
for await fraction in progressValues {
updateDisplay(fraction)
}
updateDisplayAsFinished()
/*less defensive*/
for await fraction in progressValues {
if isFinished(fraction) {
updateDisplayAsFinished()
} else {
updateDisplay(fraction)
}
}
The first one is both easier to write and confirms the eventual state.
Thanks for the feedback again, we appreciate the patience. We have been working on and will provide a package of worked examples that you can tinker with. For now, I'll address some of your questions here:
Custom Properties Aggregation
Yes, this iteration does remove the requirement to provide a reduce method in the Property protocol used to declare custom properties. In place of that, we provide a values(of:) method on ProgressManager to get all the current values of additional properties from the subtree and a total(of:) method to aggregate the current values of additional properties from the subtree. These are included in the proposal's Detailed Design section:
/// Returns an array of values for specified property in subtree.
///
/// - Parameter property: Type of property.
/// - Returns: Array of values for property.
public func values<P: ProgressManager.Property>(of property: P.Type) -> [P.Value?]
/// Returns the aggregated result of values where type of property is `AdditiveArithmetic`.
/// All values are added together.
///
/// - Parameters:
/// - property: Type of property.
/// - values: Sum of values.
public func total<P: ProgressManager.Property>(of property: P.Type) -> P.Value where P.Value : AdditiveArithmetic
The reason we redesigned it this way was because we realized that the previous design cannot, as you correctly pointed out in the previous Review thread, accommodate the dual-mode of operations while doing automatic reduction.
Issues with Previous Design (Assuming we still had the previous design)
Previously, the getter of custom properties would provide the aggregated value of custom properties in the subtree, while the setter allows us to set the property on the node itself. This surfaces the problem when we try to setfileTotalCount on node A that has children B and C with their own fileTotalCount.
Automatic reduction was great, A.withProperties(\.totalFileCount) will give us 9. However, when we try to increase the totalFileCount of A, we run into issues with the calculation:
A.withProperties { properties in
properties.fileTotalCount += 1
}
Ideally, we would want it so that A.withProperties(\.totalFileCount) gives us 10. However, because the getter of fileTotalCount returns 9 and we add 1 to it, we set totalFileCount of A to 10 and not 3. The next time we query A again, hoping to get 10, we get 17 instead because the aggregation will be 10 + 3 + 4.
While we can introduce another function to set each of the custom property, for example, something like add<P: ProgressManager.Property>(_: P.Value, property: P.Type) and make withProperties get-only, we lose the flexibility to allow developers to set multiple custom properties in a single closure.
We choose to preserve the ability for developers to write to multiple custom properties within withProperties, which leads us to the current design.
How to Work With Current Design (Based on current design)
With the current design, we would be able to set the fileTotalCount of node A without messing up the calculations. This is because in this case, the withProperties closure only mutates the custom properties of A so getting fileTotalCount of A will return 2 instead of 9, and setting += 1 on A will set fileTotalCount to 3 correctly.
To get the custom properties of the subtree with A being the root, we can call values(of: TotalFileCount.self) and get [Optional(2), Optional(3), Optional(4)]. If we want the aggregate values, we can call total(of: TotalFileCount.self), which would return 9.
Adding Aggregation to ProgressReporter
You're right about this, thanks for pointing it out. I'll add the value(of:) and property(of:) method into ProgressReporter.
ProgressManager vs ProgressReporter
ProgressReporter is the read-only version of a ProgressManager. It exposes the properties of a ProgressManager in a read-only manner, and ProgressReporter cannot be used to mutate the ProgressManager it is instantiated from. This can be found in the DetailedDesign section:
/// A `ProgressReporter` instance, used for providing read-only observation of progress updates or composing into other `ProgressManager`s.
public var reporter: ProgressReporter { get }
So instead of being two separate entities, each ProgressReporter is a representation of a ProgressManager. This is mainly to cater to a common usage pattern that we have seen on the existing Progress or NSProgress, in which a class vends a read-only property Progress at the top level. ProgressReporter allows developers to do more than that: developers can observe it, and also add be added as parts of multiple progress trees.
This additional type ProgressReporter is introduced also to support the multi-parent use case that has been mentioned in the previous Pitches and Review.
Hope this helps to clarify some things for you! Feel free to follow up with more questions if you have them :))
Yes, this was by design. I realized that we may need to account for any types of custom properties that developers might want to declare. In the case where developers want to declare a type which has an optional value, with default value being nil, the values(of:) method need to be able to return an array representing these optional values.
Unfortunately, the trade-off for preserving developers' ability to define Property.Value as an optional is that customers will "know the structure of" their subtree.
From my understanding, you're referring to the fact that the assigning count from ProgressManager to Subprogress at the time of creation for Subprogress does not work for your case because you'd like to determine what portion of its parent's totalCount the child ProgressManager should be accounted for in its parent ProgressManager.
Assigning a portion of the totalCount to Subprogress at the time of creation of Subprogress is by design. This is because we want to establish a clear separation of concerns between a ProgressManager and a Subprogress. A Subprogress is a carrier of information for instantiating a ProgressManager later. When Subprogress is consumed to instantiate a ProgressManager, developers should only have to worry about determining what the totalCount for the ProgressManager.
For example, say we have a library that has multiple methods that can be called by other developers, the library authors would have no information about what the totalCount of the ProgressManager from which it gets its Subprogress from, so they should not be able to mutate the assigned count of Subprogress.
The library code will not be aware of the fact that the overallProgress would have a totalCount of 2, so it should not be able to adjust the assignedCount of 1 to something else.
In this case, I think it can miss an update if the ProgressReporter.value is updated concurrently:
withObservationTracking called (internally calls generateAccessList)
view.body called
view.body accesses progressReporter.value, receiving 0.9 (internally, ObservationRegistrar.access is called)
whilst view.body continues executing, progressReporter.value is concurrently updated to 1.0 (internally, ObservationRegistrar.willSet and didSet are called, but nobody is registered to be notified yet)
view.body returns; the observation is established (interest in progressReporter.value) (internally _installTracking is called)
progressReporter.value never changes again, so onChange: is never called, so the view is never re-rendered
I think the AsyncSequence case has a similar problem — There's always a "gap" between generateAccessList calling its closure to figure out what we're interested in, and _installTracking actually ensuring we get notified of changes, into which a concurrent update of a property can slip.
Your response suggested:
/*defensive*/
for await fraction in progressValues {
updateDisplay(fraction)
}
updateDisplayAsFinished()
But by my understanding, the async stream of observations will not finish unless
the iterating task is cancelled (in which case we certainly don't want to claim the operation completed)
theoretically if the stream's closure accessed no properties (eg. because a weak reference has dropped), the stream could notice that the autoclosure to withObservationTracking is not called, and terminate rather than suspend until the iterating task is cancelled; it appears not to handle this case.
My question wasn't "how" but "when", how do I know that these values have changed, in order to call this method, which I think is now answered by:
And the answer is: the progress tree cannot do any intermediate aggregation; any aggregation must be done at the top level, by responding to an observation of .value by calling .values(of:) and aggregating explicitly at that time. IOW, we expect a View to look like:
struct MyView: View {
@State var progress: ProgressReporter = ...
var totalFileCount: Int {
total(progress.values(of: TotalFileCount.self))
}
var body: some View {
Text("Inspecting \(totalFileCount) Files…")
}
}
However, this won't currently work (with the PR as it stands today) because values(of:) does not establish an access on ProgressReporter, so observation will never trigger and the view will never re-render.
Thank you, but this was also promised at the last review and has yet to materialize; I think that this should be prepared before the review, as it would likely have discovered some of the shortcomings I'm trying to find by reading the code (eg. the absence of .values(of:) on ProgressReporter should definitely have shown up in examples)
Less that the child should determine the fraction, than that the parent should be able to handle discovered work.
For example, if I'm plumbing together a library which does a recursive filesystem traversal (always nondeterministic) and a library which does image processing (files should be weighted in the overall progress by their size):
func processAllImages(_ progress: consuming Subprogress?) async throws {
let manager = progress?.start(...)
await withThrowingDiscardingTaskGroup { group in
for await (path, fileSize) in traverseFilesystem(manager?.subprogress(assigningCount: ...)) {
group.addTask {
try await processImage(path, manager?.subprogress(assigningCount: ...))
}
}
}
}
how should I fill those ellipses, in such a way that:
my UI can report that filesystem traversal is still occurring (eg. with a spinner or a label showing which paths it's inspecting)
the deterministic progress is (roughly) linear in the number of bytes sent to processImage
the progress completes when the filesystem traversal is complete and all the images have been processed
I think the existing design would support:
func processAllImages(_ progress: consuming Subprogress?) async throws {
let manager = progress?.start(1)
var filesToProcess = [(String, UInt64)]()
for await (path, fileSize) in traverseFilesystem(manager?.subprogress(assigningCount: 0)) {
filesToProcess.append((path, fileSize))
}
let totalByteCount = filesToProcess.reduce(0) { $0 + $1.1 }
let processingManager = manager?.subprogress(assigningCount: 1).start(totalCount: totalByteCount)
await withThrowingDiscardingTaskGroup { group in
for (path, fileSize) in filesToProcess {
group.addTask {
try await processImage(path, processingManager?.subprogress(assigningCount: fileSize))
}
}
}
}
But that doesn't let me interleave discovery with processing, which I absolutely need to be able to do for performance.
I see, in this case, if you're referring to parent handling discovered work by adjusting the portion of its totalCount assigned to its children, we don't allow parent ProgressManager to adjust the portion of totalCount assigned because of two concerns:
Making sure that progress does not go backwards
If we were to allow developers to adjust the assignedCount given to its children in all cases, we might risk letting observers of UI see that the progress bar might go backwards instead of forward at all times.
Clearer separation of concern between ProgressManager and Subprogress, mentioned in a previous comment here:
And on to the use case you've laid out here, if I do understand this correctly, here are my suggestions. Feel free to also follow up if there are some things that I am missing!
If you'd like to make sure that your UI shows that progress is indeterminate while filesystem traversal is still occurring, you can set totalCount of manager to nil in the beginning. So that you show that the overall progress shows an indeterminate spinner on the UI. While the filesystem traversal is happening, you can set a custom property to show which path is currently being inspected.
The API does allow you to create a Subprogress with an assignedCount that is non-zero from a ProgressManager that has totalCount of nil, so you can preserve the indeterminate status until you discover the totalByteCount at a later date. This is shown in Example 1 of the package linked above.
While it would be ideal for progress to be roughly linear in the number of bytes sent to processImage, we also do not encourage for byteCount to be used as totalCount because updating byte-by-byte can be non-performant. Instead, in this case, I would suggest maybe using the number of paths as the totalCount, and use the additional property totalByteCount to represent total number of bytes.
the deterministic progress is (roughly) linear in the number of bytes sent to processImage
Can you help me understand this part?
Let's say I traverse the file system, and I have 3 images.
A (1 byte)
B (1 byte)
C (8 bytes)
Even if this were async, let's assume it happens in order. What do you expect the percent completed of the overall progress to be when we've discovered (and processed) A & B, but not C yet? Is it 66%, but then goes backwards to 20% after we discover C (and haven't finished processing it yet)?
edit: Actually, I'm not even sure we know it would be 66% because we don't know C exists yet. So is it 100% because A and B are done?