refactor(skore/progress): Add comments and simplify#2032
refactor(skore/progress): Add comments and simplify#2032auguste-probabl wants to merge 6 commits intoprobabl-ai:mainfrom
Conversation
The test applies to things that have a `reports_` attribute, but in reality the only things `progress_decorator` applies to are skore.*Report and their associated Accessors.
| if hasattr(self_obj, "reports_"): | ||
| if isinstance(self_obj, ComparisonReport): |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
Sorry for the delay. I think to not have something tight to the ComparisonReport would make it better.
| # assigning progress to child reports | ||
| # Make child reports share their parent's Progress instance | ||
| # so that there is only one Progress instance at any given point | ||
| reports_to_cleanup: list[Any] = [] |
There was a problem hiding this comment.
Thinking a bit more about the problem, I think that it would be cleaner to have a parameter in the decorator (i.e. clean_child_reports) that would trigger the cleaning. It means that we call in the ComparatorReport, the decorator with clean_child_reports=True that trigger this code without having to check the type of report. For the other report, then we need to pass clean_child_reports=False.
There was a problem hiding this comment.
The point of this PR is to reduce progress_decorator's complexity (which we have been bit by many times now) by recognizing that it is only used within *Report objects, and making it less general.
In other words, what I mean is, the proposed parameter is really just another way of asking "is the report a ComparisonReport", and I'd rather just ask that question directly.
There was a problem hiding this comment.
What I don't like here is the fact that we import ComparisonReport.
Could we instead create a report._report_type that is aligned with the other case, for instance when calling _compute_data_for_display that takes report_type (
If we have this attribute for each type of report, we only need to propagate it instead of hard-coding it. It means that here we only need to have if "comparison" in self_obj.report_type.
But otherwise, I'm fine with the rest.
| assert task._progress_info is None | ||
|
|
||
|
|
||
| def test_child_report_cleanup(): |
There was a problem hiding this comment.
With my proposal then we can test the procedure because we are not tight anymore to a ComparisonReport instance specifically.
This comment was marked as resolved.
This comment was marked as resolved.
auguste-probabl
force-pushed
the
push-xwnvukqxwvmp
branch
from
282a822 to
fb74952
Compare
|
Obsoleted by #2357 |
3 participants
In particular, make it clearer that
progress_decoratoris very specific toskore.*Reports