-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MNT: refactor model card to render sections lazily #310
Merged
adrinjalali
merged 7 commits into
skops-dev:main
from
BenjaminBossan:MNT-refactor-model-card-lazy-sections
Mar 7, 2023
Merged
MNT: refactor model card to render sections lazily #310
adrinjalali
merged 7 commits into
skops-dev:main
from
BenjaminBossan:MNT-refactor-model-card-lazy-sections
Mar 7, 2023
Commits on Mar 2, 2023
-
[WIP] Refactor Card to lazily format sections
Currently, the way model cards work is that if a user adds new content, a Section object is created and added as a (sub)section. That object contains the *formatted* content of that section as str, which is then used when rendering. So if the user adds, e.g., a table, the table is eagerly formatted to a str and during rendering, the str is simply returned. With this refactor, when a user adds new content, a specific type of section is added instead, which does not eagerly format the string. For simple text content, this is just a PlainSection which holds the content as a str, so no big change. However, for more complex content like tables or figures, a special section type is added that does not contain the final content as rendered. When the model card is rendered, the section.format() method is called to render the actual content, i.e. the formatting is now called lazily. The main benefit of this refactor is that the sections in a model card are now "aware" of what type of section they are. This allows us to more easily modify them later. E.g. let's say we want to modify the path to a figure or a row in a table: Right now, this would require some quite complex regex to modify the already formatted str. With this refactor, we can just access the attributes of the section, e.g. section.path or section.table, and modify them. A minor benefit could also be performance, because we don't render until it's strictly needed, but that's probably very minor. From the user perspective, this refactor shouldn't change anything as long as they were sticking with the official API. When calling render() or save(), the output is the same as before. Only if the users were directly interacting with sections might there be a breakage. One minor visible change this PR brings as well is that the repr of the card changes slightly. Now it indicates what type of section we're dealing with, e.g. TableSection(...), except if it's a PlainSection, which, same as previously, just returns the repr of its content. This commit is WIP because I also added new "description" parameters to add_* methods that were missing it so far. Now, if a user adds, say, a plot, they can optionally add a description for the plot too. This functionality lacks unit tests as of now.
Configuration menu - View commit details
-
Copy full SHA for 85503ed - Browse repository at this point
Copy the full SHA 85503edView commit details -
Configuration menu - View commit details
-
Copy full SHA for d2585a0 - Browse repository at this point
Copy the full SHA d2585a0View commit details -
Configuration menu - View commit details
-
Copy full SHA for 0669b28 - Browse repository at this point
Copy the full SHA 0669b28View commit details
Commits on Mar 3, 2023
-
Add description param to permutation importances
The add_permutation_importances method now also has a description argument. I also reworked the tests for permutation importances a bit: - added a new test for description - put all related tests inside a test class - moved the permutation importance computation into a fixture - moved to the new testing method of selecting section and checking its content exactly instead of checking a substring in model_card.render()
Configuration menu - View commit details
-
Copy full SHA for 833aced - Browse repository at this point
Copy the full SHA 833acedView commit details -
Now, Section is a concrete class (taking the role of PlainSection).
Configuration menu - View commit details
-
Copy full SHA for 87547df - Browse repository at this point
Copy the full SHA 87547dfView commit details -
Configuration menu - View commit details
-
Copy full SHA for 18c0855 - Browse repository at this point
Copy the full SHA 18c0855View commit details
Commits on Mar 6, 2023
-
Configuration menu - View commit details
-
Copy full SHA for b66db87 - Browse repository at this point
Copy the full SHA b66db87View commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.