Skip to content

Added DataclassPPrintMixin#92

Closed
MischaPanch wants to merge 1 commit intodevelopfrom
util/dataclass-pprint-mixin
Closed

Added DataclassPPrintMixin#92
MischaPanch wants to merge 1 commit intodevelopfrom
util/dataclass-pprint-mixin

Conversation

@MischaPanch
Copy link
Collaborator

We use it in tianshou, I'd also like to use it in releal

@MischaPanch MischaPanch requested a review from opcode81 August 1, 2025 11:21
@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

Why is this needed? Inheriting from ToStringMixin already provides pprint functionality and also elaborate inclusion/exclusion mechanisms. The only thing this does differently is that it presents it as a dictionary instead of the original type, and it's not clear to me why this would be a good thing.

@MischaPanch
Copy link
Collaborator Author

I explained in the docstring of the class - here you can choose which fields to exclude during calling instead of during implementation. This was needed quite often in the past during debugging

@MischaPanch
Copy link
Collaborator Author

Btw, the usage in tianshou is broken - Experiment is no longer a dataclass, so the mixin methods don't work (raise exceptions). Pushing a fix to main where the inheritance of this is removed in Experiment

@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

This is also possible with ToStringMixin if you implement a very simple wrapper around _tostring_properties. Something like this (untested):

def pprints_as_dict(self, exclusions):
   return "{" + self._tostring_properties(exclude=exclusions) + "}"

@MischaPanch
Copy link
Collaborator Author

Sure, this is a better way to achieve this. Gonna adjust

@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

Btw, the usage in tianshou is broken - Experiment is no longer a dataclass, so the mixin methods don't work (raise exceptions). Pushing a fix to main where the inheritance of this is removed in Experiment

The dataclass cannot inherit from anything, because this breaks all examples. It is documented in the commit!

@MischaPanch
Copy link
Collaborator Author

I'm saying Experiment should not inherit from DataclassPprintMixin, not that Experiment should become a dataclass

@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

Note that the function I sketched above should not be added to ToStringMixin, because it goes against the principle of ToStringMixin using a fixed configuration, i.e. it results in vastly different behaviour between pprints and pprints_as_dict, which is not desirable. If it is added to a particular class, it should have a very different name to make clear that is not using the configured string conversion settings at all.

@MischaPanch
Copy link
Collaborator Author

How about PPrintsWithExclusionsMixin?

@MischaPanch
Copy link
Collaborator Author

Anyway, not urgent, closing this for now. Seems like we won't be using the mixin anywhere soon (I was just porting Experiment, but it shouldn't have been used there anyway). We can keep it in the back of our heads, eventually remove the DataclassPprintMixin from tianshou

@MischaPanch MischaPanch closed this Aug 1, 2025
@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

I am still not convinced that there truly is a use case for this.
Why is it really needed?
For every type of object, there is a well-defined set of things that are relevant, and they should be included - always.
If, for some debugging task, you need only a few pieces of information, then why not print these explicitly - adapted for your debugging task. If you don't need just a few pieces of information, then why not print everything and ignore the rest of the output? I currently don't think that the edge case this seems to be about really needs to be addressed.

@MischaPanch
Copy link
Collaborator Author

I had used this on objects containing general info as well as some fields with large arrays. I want to see everything except the arrays. When printing everything, the arrays pollute the output. At the same time, I don't want to print each field individually (there were many). That's the use case

@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

Long-ish arrays should never be part of string representations. Short-lived debugging tasks that need to print them should just print them explicitly imo. If need be, one could add a function that explicitly prints the detail info that is normally left out.
So I would approach this from the opposite direction, perhaps.

@opcode81
Copy link
Collaborator

opcode81 commented Aug 1, 2025

So to make it fit into the ToStringMixin concept, an interface that could make sense would be something like

pprints_extended(ignore_all_exclusions: bool, forced_inclusions: list[str])

@MischaPanch
Copy link
Collaborator Author

Yeah, that sounds good! Let's do this when the need arises, in the spirit of XP :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants