MAINT Rendering the model card #94
Conversation
Exactly. Okay, then I'll finish the refactoring. |
- Add missing docstrings - Add entry to changes.rst - Simplify all tests that can use render
|
@skops-dev/maintainers Ready for review. |
BenjaminBossan
changed the title
| ---------- | ||
| path: str, or Path | ||
| filepath to save your card. | ||
| def _generate_card(self) -> ModelCard: |
There was a problem hiding this comment.
This doesn't really generate the card itself because it already exists, but generates the content to be written. That would be better to go for a better naming IMHO.
There was a problem hiding this comment.
Yes, it doesn't generate the skops Card, it generates the modelcards ModelCard object. What would be a better name? I think _generate_content is a bit vague. I'm also not attached to generate, we can also use a plain make or something.
There was a problem hiding this comment.
Since it's not a public method we could improve the docstring instead, I guess. Same with the below one.
There was a problem hiding this comment.
Happy to improve the docstring. Could you please let me know what I should clarify -- since I made the change, for me it's obvious ;)
Same with the below one.
Which one do you mean?
| card.save(path) | ||
|
|
||
| def render(self) -> str: |
There was a problem hiding this comment.
I liked what you did here but it's actually _generate_card that does rendering (with from_template) so I'd call this something else like render_to_str to avoid confusion for future maintainers. Feel free to ignore if you don't agree 🙂
There was a problem hiding this comment.
but it's actually _generate_card that does rendering (with from_template)
I agree that _generate_card does the heavy lifting, but from a user perspective, it doesn't really matter.
so I'd call this something else like render_to_str to avoid confusion for future maintainers
I did think about render_to_str. IMO the name should be chosen with respect to the users, not so much maintainers, as it'll be a public method. render_to_str makes sense if we plan on adding other outputs and then have render_to_foo. If, instead, we want to support those other formats via arguments (card.render(format="foo")), I'd stick with the terse render. What is the more likely scenario?
There was a problem hiding this comment.
I agree with @BenjaminBossan on how the methods are organized here. _generate_card is not something we'd document for users and they should never call it. So from their perspective, there's only render and save.
| card = self._generate_card() | ||
| card.save(path) |
There was a problem hiding this comment.
You mean call self.render and then write the string directly to the file, instead of going through card.save?
There was a problem hiding this comment.
yeah that's a good question. I'm wondering if there could be a difference between users calling save and saving the output of render, which wouldn't be ideal I think. WDYT?
There was a problem hiding this comment.
At the moment, both would amount to the same, but who knows what else could be added on the modelcards side in the future. Therefore, I would prefer users to use skops Card.save, which in turn should use ModelCard.save. But if a user wants to call render and save that, I guess we can't prevent them.
There was a problem hiding this comment.
LGTM. Merge conflicts though.
I'd wait maybe for @merveenoyan to have a final look.
|
Solved the merge conflict and wait for Merve's verdict. |
|
Merging this one. @merveenoyan feel free to open an issue or a PR fixing things which need to be fixed when you're back :) |
I wanted to discuss with you @skops-dev/maintainers what they think about this refactoring. Right now, the
Card.savemethod performs the whole job of actually generating theModelCardobject. My plan is to simply factor this into a new method,_generate_card, and have thesavemethod call this instead.The main reason is to separate the concerns, i.e.
saveis only responsible for saving, not generating. What this affords us is that we can now easily add another method,render, which returns the generated model card as a string. This can, for instance, be used in the tests, as I have shown with one example. Instead of generating the card, storing it in a temp folder, reading it, then checking it, we can now justrenderthe string and check it directly.Please LMK if you agree and I will finish this PR.