FIX Fix issue with setting model_diagram to False #314
Conversation
As discussed in skops-dev#296, instead of dealing with the issue what should happen if the user sets model_card.model_diagram = False after initializing with True, just remove the attribute from the Card class. This way, there cannot be the false impression that the diagram can be removed from the card by setting the attribute to False. If users really need to remove the model diagram after enabling it at first, they can just delete the corresponding section (or make it invisible). On top of that, a feature was added that passing a str to model_diagram will use that str as the section name. Previously, always the default section would be used (for skops template) or no section added (for custom templates).
|
@skops-dev/maintainers Ready for review (I can't re-trigger docs build, which failed because of huggingface_hub 0.13.0). |
skops/card/_model_card.py
Outdated
| Set to True if model diagram should be plotted in the card. | ||
| model_diagram: bool or str, default=True | ||
| If using the skops template, setting this to ``True`` will add the model | ||
| diagram, as generated by sckit-learn, to the default section. Passing a |
There was a problem hiding this comment.
I think we should mention what the name of the section is, for them to be able to select and modify it later.
skops/card/_model_card.py
Outdated
| If model_diagram is truthy and str, use str as section name. If | ||
| model_diagram is True, use default section for default template or | ||
| ignore for non-default template. If model_diagram is False, don't | ||
| add it. |
There was a problem hiding this comment.
I think this logic makes more sense (and update the code accordingly)?
| If model_diagram is truthy and str, use str as section name. If | |
| model_diagram is True, use default section for default template or | |
| ignore for non-default template. If model_diagram is False, don't | |
| add it. | |
| If model_diagram is str, use it as section name. If | |
| model_diagram is True, use default section for default template or | |
| raise for non-default template. If model_diagram is False, don't | |
| add it. |
There was a problem hiding this comment.
If model_diagram is True ... raise for non-default template
That's what I started with. The problem is that if I initialize a Card object with a custom template, I would immediately get the error and need to also set model_diagram=False. I would find this very annoying as a user, since I never asked for the model diagram. I would prefer it if I could initialize the Card and only get the error if I explicitly call model_card.add_model_plot (same behavior as now).
There was a problem hiding this comment.
the solution to that would be to change the default of model_diagram to "auto", which would be True if the default template is used, and False otherwise.
There was a problem hiding this comment.
I could do that, but I wonder if this isn't overly complicated for this edge case. Especially if we want to allow passing arbitrary strings to model_diagram, having a special case for "auto" is a bit odd.
There was a problem hiding this comment.
I'd say using "auto" as a section name is not wise in a model card anyway ;)
There was a problem hiding this comment.
Done (waiting for the first bug report because a German wrote a car ML model).
"auto" is the new default and it will add a model_diagram only if the default template is used, preserving the current behavior.
|
@skops-dev/maintainers Addressed comments, ready for review |
skops/card/_model_card.py
Outdated
| model_diagram: bool or "auto" or str, default="auto" | ||
| If using the skops template, setting this to ``True`` or ``"auto"`` will | ||
| add the model diagram, as generated by sckit-learn, to the default | ||
| section. Passing a string to ``model_diagram`` will instead use that |
There was a problem hiding this comment.
We should add the section name here.
| section. Passing a string to ``model_diagram`` will instead use that | |
| section, i.e. {SECTION_NAME}. Passing a string to ``model_diagram`` will instead use that |
adrinjalali
changed the title
Resolves #292, supersedes #296. The discussed solution was sufficiently different from the one proposed in #296 that it was better to open a new PR than to rework that one.
The new solution is the following: Instead of dealing with the issue what should happen if the user sets
model_card.model_diagram = Falseafter initializing withTrue, just remove the attribute from theCardclass. This way, there cannot be the false impression that the diagram can be removed from the card by setting the attribute toFalse.If users really need to remove the model diagram after enabling it at first, they can just delete the corresponding section (or make it invisible).
On top of that, a feature was added that passing a str to
model_diagramwill use that str as the section name. Previously, always the default section would be used (for skops template) or no section added (for custom templates).