FIX: setting model_diagram to False after initialization#296
FIX: setting model_diagram to False after initialization#296BenjaminBossan wants to merge 2 commits intoskops-dev:mainfrom
Conversation
Resolves skops-dev#292 When setting model_card.model_diagram = False, the rendered model card should not contain the model diagram. For more explanation, see the issue.
|
Ready for review @skops-dev/maintainers |
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
Fixing the comment, I think would also make this PR fix #292
| # If we use the skops template, we know what section to add or remove | ||
| # when model_diagram changes values. If not, we don't know and thus need | ||
| # to skip this step. | ||
| if self.template != Templates.skops.value: | ||
| return |
There was a problem hiding this comment.
instead of this, we can raise and say that we don't know what to do about this.
There was a problem hiding this comment.
As in, instead of returning here, we could raise an error, and if we do, then this PR can be considered to fully fixing the original issue IMO.
There was a problem hiding this comment.
I forgot about this PR, hence the late reply :)
I changed the code to raise an error here, with a hopefully useful error message for the user.
This is still not solving the whole problem though. We have test_setting_model_diagram_false_other_section, which is still xfailing. The problem is that even with a skops template, if the user adds a model diagram to a different section, since we don't store that section name, we cannot remove it later on. For that, I see two possible solutions, but I don't like either:
- Don't allow users to add a model diagram to a different section when they have the skops template (why would they need it twice?) -- but should we really disallow it?
- We remember where we put stuff, e.g. saving an attribute
_section_name_of_model_diagram, but that seems to be overly complex for such a niche error.
WDYT?
There was a problem hiding this comment.
So this makes me realize maybe our abstraction is a bit the issue. We're trying to handle metadata related to the template, but not stored in the template. One way to fix this, would be to allow users to set attributes on the template, such as "collapsed/expanded" for each section. We can still accept a dictionary as a template, but if the template extends the dictionary and includes such information, we apply them to the rendered readme. WDYT?
There was a problem hiding this comment.
I agree that this smells of incorrect abstraction. But I'm not sure if the proposed solution is the right one. Right now, the template is used to prefill the model card, not anything more. Let's assume we would allow users, as you suggest, to set "collapsed/expanded" on the template. If they use the template that way, and later add a new section, how would they control if that section is collapsed? As it's not in the template, they would have to set it on the card. Now there are two different ways to achieve the same thing.
I think the problem in the discussed case is the conflict between flexibility&customization vs predefined sections. To accommodate the former, we have to allow 0, 1, or multiple model diagrams anywhere in the card. For the latter, we have to assume there either is or isn't a model diagram, and it always sits in the same place. I don't have a good proposal to resolve that conflict.
Regarding this specific problem, it might be better to not have a model_diagram=True/False argument on Card at all. Instead, just never create it automatically, and if I need it, I have to call model_card.add_model_plot(section_name) (the section_name argument would be optional with the skops template). If I don't want it anymore, I call model_card.delete(section_name). IMO that is much cleaner, but it's breaking BC.
There was a problem hiding this comment.
I agree with most of what you say. Here's another solution:
- don't treat model diagram as a special case. Create a [sub]section for it, and put the diagram content there.
- we don't have to remove the arg from
init, but we can make it bemodel_diagram=True/None/section name - if the user wants to hide the model diagram, they should hide the section
The semantics of the following code becomes:
card = Card(..., model_diagram=None) # don't add the diagram yet
card.add_model_diagram(section_name) # now add the diagram
card.select(section_name).hidden = True # hide the model diagramIs that better?
There was a problem hiding this comment.
Sounds doable. For clarification:
if the user wants to hide the model diagram, they should hide the section
Is your suggestion that if the user sets model_card.model_diagram = False, we should try to make it invisible (this PR) or just do nothing (status quo)?
There was a problem hiding this comment.
We can remove the boolean model_diagram from the object. The object would either not have it at all, or if it does, it'd be the Section object.
Ah, sorry, I don't follow. Is this referring to your other remark:
? |
That's better than just ignoring it, especially if the error message can guide the user to the right solution. Also, early return in setter in case the value stays the same.
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).
|
Closing in favor of #314 |
2 participants
Resolves #292
When setting
model_card.model_diagram = False, the rendered model card should not contain the model diagram. For more explanation, see the issue.The issue is not resolved for the case that a user doesn't use the skops template, because it means we don't know where they put the model diagram and can thus not disable it after they set
model_card.model_diagram = False. The corresponding test is marked toxfail.The test suite has been extended to cover more cases around the use of custom templates in conjunction with the model diagram.