[2/N] Define dataclasses for progress tracking

[carmocca]

What does this PR do?

Continues #6603
Part of #6429

The following google document documents the purpose for each of these attributes

https://docs.google.com/document/d/1U73b4uFML2MNdObh6MVsBpFDrgLUOU2Sd_x0_DVk-Bo/edit?usp=sharing

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

[codecov]

Codecov Report

Merging #7574 (8ffcaf7) into master (8266b14) will decrease coverage by 0%.
The diff coverage is 90%.

@@           Coverage Diff           @@
##           master   #7574    +/-   ##
=======================================
- Coverage      92%     92%    -0%     
=======================================
  Files         198     198            
  Lines       12930   12861    -69     
=======================================
- Hits        11955   11843   -112     
- Misses        975    1018    +43     

[ananthsub]

this means at construction we need to null out the fields we want to mark as unused. indicating this API behavior change based purely on types is not intuitive. this also means if anyone extends this class and needs an optional field which is initially None but later tracked for their loop, they also need to override __setattr__

from the gdoc, batch/epoch can have reasonable definitions for each of these based on the existing loop structure. optimization progress having unused fields but being forced into the same struct doesn't seem right to me. since these are brand new structs, should we instead offer per-use case structs that contain only the fields we need?

[carmocca]

indicating this API behavior change based purely on types feels is not intuitive.

I agree it's not the clearest but to me seems like an okay tradeoff to avoid the code duplication.

this also means if anyone extends this class

I don't think there is a reason to extend the Progress class in particular. BaseProgress and so on might be extended but they don't contain this "unused" logic.

since these are brand new structs, should we instead offer per-use case structs that contain only the fields we need?

would it be easier to introduce a separate optimization progress struct which doesn't carry all these fields, and if there is demand for them later, then we can unify using the same struct?

I'm open to it, but do you have an idea how? Because this wouldn't work well:

@dataclass
class Progress:  # name?
    ready: int = 0
    started: int = 0
    processed: int = 0
    completed: int = 0


@dataclass
class Progress1:  # name?
    ready: int = 0
    started: int = 0
    completed: int = 0


@dataclass
class Progress2:  # name?
    ready: int = 0
    completed: int = 0

@dataclass
class OptimizationProgress:
    optimizer: BaseProgress = BaseProgress(total=Progress1(), current=Progress1())
    scheduler: BaseProgress = BaseProgress(total=Progress2(), current=Progress2())
    zero_grad: BaseProgress = BaseProgress(total=Progress1(), current=Progress1())

There are several issues with this structure:

• Duplicated docstrings between classes
• Duplicated helper functions between classes
• Typing complaints as there is no shared class
• BaseProgress assumes the 4 attributes are available

do we think optimization progress would use these fields later?

Maybe. I couldn't think of a use for them but they could be useful in the future.

[tchaton]

Looks great !

I think the design is pretty elegant to avoid code duplication and improve code readability.

@ananthsub Let's get this in, so we can actually start working on the refactor. Right now, this PR is blocking us.

[ananthsub]

My preference is to revisit this design if/when we have a cleaner way to separate the tracker structs to avoid using Optional like this.

However since we don't have that now, accepting to unblock