FEAT Parse model cards with pandoc #257
Conversation
This is a suggestion to address the issues discussed on skops-dev#72 Description The proposed model card implementation would allow to dynamically add sections or overwrite them. This is not a complete implementation but already covers most of the features we already have and then some. On top of these features, it would be possible to add more features like creating a default Card with placeholders, just like the exisint template, or the possibility to delete existing sections or to retrieve the result of a certain section. Implementation The underlying data structure consists of a dict and a Section dataclass. All data is stored in a _data attribute with the type dict[str, Section]. The dataclass hold the section contents, i.e. the section title, the section content, and subsections, which again have the same type. It's thus recursive data structure. Section title and dict key are identical, which is mostly for convenience. With this refactor, there are no separate data containers anymore for eval results, template sections, extra sections, etc. They are all treated the same. IMHO, this greatly simplifies the code overall. The only complex function that's left is the one needed to traverse the tree holding the data, and even that is just 14 LOC. Demo To see how the new class can be used, take a look at the main function. The resulting Card can be seen here: https://huggingface.co/skops-ci/hf_hub_example-fcc0d6fe-d072-4f94-8fdb-6bf3bb917bca
Added a test that shows that the new card produces the same output as the old card (except for a few non-deterministic parts). This includes most of the idiosyncrasies of the old card we might want to change in the future (e.g. inconsistent capitalization, use of empty lines). Some of the more problematic behaviors of the old card class were, however, fixed (e.g. creating an empty metrics table when there are no metrics). The other tests have been reworked to use the new card features to make them more precise. Often, that means that instead of having a very weak test like "assert 'foo' in card.render()", it is now possible to select the exact section and check that it equals the expected output. This work is still unfinished, specifically it still lacks tests for the card repr and for the newly added features.
Some refactoring to clean up things, rework repr, make repr tests pass.
Also some better type annotations.
- Remove noise from docstring example - Add the comma after model repr - Add docstrings to private methods
Users can now choose to use no template, skops template, hub template, or their own template. Using their own template disables a lot of prefilling (say, putting the model plot in the card) because we wouldn't know where to put it. Users will need to call card.add for the otherwise prefilled sections.
This can be useful, because otherwise it takes a bit of effort to retrieve the latest section.
It's ugly, but there is no technical reason from prohibiting the addition of tables without rows. (Note, columns are still required). This allows us to use TableSection for formatting the metrics, instead of calling tabulate there directly. This is better, since we don't have 2 separate ways of creating metrics.
As discussed here: skops-dev#72 (comment) Description This feature adds a new function, skops.card.parse_modelcard. When passing it the path to a dumped model card, it parses it using pandoc and returns a Card object, which can be further modified by the user. In the end, this turned out easier than I initially thought it would. The main difficulty are the data structures returned by the pandoc parser, for which I couldn't find any documentation. I guess Haskell code is just self-documenting. For this reason, there are probably quite a few edge cases that I haven't covered yet. Just as an example, when parsing tables, pandoc tells us how the columns are aligned. This information is currently completely discarded (we let tabulate choose the alignment). If we want to preserve the table alignment, we would need to make some changes Implementation This feature requires the alternative card implementation from skops-dev#203 pandoc is used for the following reasons: - widely used and thus battle tested - can read many other formats, not just markdown, so in theory, we should be able to read, e.g., rst model cards without modifying any code The disadvantage is that pandoc is not a Python package, so users need to install it separately. But it is available on all common platforms. For calling pandoc, I chose to shell out using subprocess. I think this should be fine but LMK if there is a better way. There is a Python package that binds pandoc (https://github.com/boisgera/pandoc) but I don't think it's worth it for us to add it, just to avoid shelling out. The package seems to have low adoption and contains a bunch of stuff we don't need. I chose to implement this such that the parser that generates the Card object should not have to know anything about Markdown. Everything related to Markdown is moved to a separate class in _markup.py. In an ideal world, we would not have to know anything about markdown either. Instead the Card object shoud have methods (similar to what we already have for add_plot etc.) that handles all of that. But in practice, this is far from being true. E.g. if a user wants to add bold text, there is no special method for it, so they would need to add raw Markdown. The Card class is thus a leaky abstraction. TODOs This PR is not finished. Remaining TODOs that come to mind: 1. We need to merge the alternative card implementation 2. Documentation has to be updated in several places 3. Tests need to be more complex, right now only one Card is tested 4. CI needs to install pandoc so that the tests are actually run 5. There are some specifics here that won't work with all Python versions, like the use of TypedDict.
Most model cards have a yaml section at the top. It is now detached before parsing with pandoc, then re-added afterwards. Add a test with the model card from bert-base-uncased. It still fails with some minor issues at the moment (most notably table column alignment).
Now supports: - Space - Strong - Emph - Strikeout - Subscript - Superscript - Plain - Str - RawInline - RawBlock - SoftBreak - LineBreak - Para - Header - Image - CodeBlock - Code - Table - Div - Link - BulletList - Quoted
I added 5 model cards from the top 10 most used models from the Hub (I excluded cards that were too similar to one another). The tests were rewritten so that they should now pass. There are some limitations to the parser that results in the generated cards not being 100% identical. Those limiations are now documented. However, I don't believe that those limitations matter, as they make no semantic difference but rather are stylistic or even invisible. The most notable difference is the alignment of columns in tables. The tests pass despite those differences because I rewrote them to include a diff file for each model card. When the generated card is compared to the original one, a diff is created and compared to the checked in diff. This way, we have control over what diff we permit. I had to exclude the folder containing the cards and diffs from the pre-commit task "trailing-whitespace", as we need the trailing whitespace in there.
There was a change in pandoc, this now works with the latest pandoc version.
For some reason, the order of items in the metainfo is no longer stable. Therefore, the tests comparing the parsed card vs original card failed. Now metainfo is excluded when comparing the cards. The metainfo is now checked separately, in a way that disregards the order.
It contained remnants from an old test that has been removed since then.
|
@skops-dev/maintainers I think this is now ready for review. For the full description, please look at the original comment. More as a meta comment, I know this will probably not be an easy review. However, I think since this is more of a "side feature" and doesn't touch the skops "core", it is okay if it isn't perfect. There should be no problem making changes later on without breaking BC, e.g. by supporting a bigger subset of markdown features. The testing is done in a kinda simple fashion, where I take a model card markdown, parse it, dump it again into markdown, then compare the diff to the original card. If the diff corresponds to the expected diff, the test passes. The test module should hopefully contain enough comments to explain these steps. |
Was added to v0.4 but that version is already released.
skops/card/_parser.py
Outdated
| # not illegal in markdown, but we don't handle it yet. | ||
| raise ValueError( | ||
| "Trying to add content but there is no current section, " | ||
| "this is probably a bug, please open an issue on GitHub" |
There was a problem hiding this comment.
| "this is probably a bug, please open an issue on GitHub" | |
| "this is probably a bug, please open an issue on GitHub." |
not sure why you like to miss the dots at the end of exception messages :D
There was a problem hiding this comment.
Not sure, probably because Python error messages also omit the period when it's only one sentence?
skops/card/_parser.py
Outdated
| def _check_version_greater_equal( | ||
| version: Sequence[int], min_version: Sequence[int] | ||
| ) -> None: | ||
| """Very bad version comparison function to ensure that the first version is |
There was a problem hiding this comment.
why not use packaging.version instead?
There was a problem hiding this comment.
We had a similar discussion in skorch: skorch-dev/skorch#887
We decided not to use packaging because we didn't want to have a dependency on it. It seems that in skops, it is available (probably a transient dependency), but it's not an explicit requirement. If you are okay with explicitly depending on packaging, I can make the change.
There was a problem hiding this comment.
setuptools depends on it, I don't think packaging is a heavy dependency, I'm happy to include it as an explicit dep.
skops/card/_parser.py
Outdated
| # not illegal in markdown, but we don't handle it yet. | ||
| raise ValueError( | ||
| "Trying to add content but there is no current section, " | ||
| "this is probably a bug, please open an issue on GitHub" |
There was a problem hiding this comment.
Not sure, probably because Python error messages also omit the period when it's only one sentence?
skops/card/_parser.py
Outdated
| def _check_version_greater_equal( | ||
| version: Sequence[int], min_version: Sequence[int] | ||
| ) -> None: | ||
| """Very bad version comparison function to ensure that the first version is |
There was a problem hiding this comment.
We had a similar discussion in skorch: skorch-dev/skorch#887
We decided not to use packaging because we didn't want to have a dependency on it. It seems that in skops, it is available (probably a transient dependency), but it's not an explicit requirement. If you are okay with explicitly depending on packaging, I can make the change.
Add packaging as an explicit dependency. Kinda arbitrarily set min version to be 17.0, which was released 5 years ago.
|
@adrinjalali I went with your (btw, yesterday pandoc 3.0 was released, I hope there is no breakage D:) |
|
So what I'm concerned about, is that when users are recommended to install a But I guess we don't want to deal with that, and that only applies to ubuntu. So I'm fine with going back to what you had in the CI. |
Apt repo is very outdated.
Yes, I totally agree. However, I feel this is more of an issue with pandoc distribution in general and not so much for us. For CI, I think we can live with the deb solution. We could consider recommending a different way of installing pandoc in our docs, but I'm not sure if we should deviate from the officially recommended method. |
Figure was added in Pandoc v3.0 and is more complex than Image, but also more powerful. At the moment, we treat Figure just as Image.
skops/card/_markup.py
Outdated
| content = f"" | ||
| return content | ||
|
|
||
| def _figure(self, value) -> str: |
There was a problem hiding this comment.
seems like the whole _figure is not tested.
There was a problem hiding this comment.
Yep, Figure is a new type that was added in pandoc 3.0. I didn't want to switch to 3.0 for CI because it is freshly released, so probably almost no one is using it. And having pandoc 2.x + 3.x in the CI matrix seemed like overkill to me.
In general, that SGTM. Just for me understanding: The idea would be that if we know that a line is not covered, we add this so that codecov et al don't complain about this line, and in order for us to signal that we are aware that this line is not tested? Would that be for all of skops or for this feature specifically? I think there is a danger of using the pragma: IIUC, if we do end up covering the line (even if unintentionally) by adding a test, we would not get an error that the line is covered now even though we "claim" it's not covered. Then, if someone else comes and, say, modifies the test so that the line no longer is covered, we would not get a warning about decreased coverage, right? So I feel like using the pramga would weaken the reason of tracking code coverage in the first place. (This is on contrast, e.g., to using pytest with |
|
If we write a test that covers those lines, we'd know, right? They're mostly raising exceptions which we'd be catching in the tests, and therefore we'd be removing the pragma. To me this is very different than The point of codecov is not to make sure every single line is covered. It's about us knowing if things which we should be testing are tested or not, and it's doing that for us. |
Those are things that don't really make sense to be tested.
- test error when trying to use markup != markdown - test no version check for pandoc - test min pandoc version too low - test pandoc not installed - test pandoc version cannot be determined
|
@adrinjalali I added pragmas where it makes sense and tests for all the remaining lines that were not tested.
Yes, for these specific lines, that's true.
I only brought that up in the sense that it has the |
adrinjalali
changed the title
As discussed here:
#72 (comment)
For maintainers: No need to review anything, I just updated this branch after merging the alternative model card implementation and pointed the PR to the main repo instead of my fork.
Description
This feature adds a new function,
skops.card.parse_modelcard. When passing it the path to a dumped model card, it parses it using pandoc and returns aCardobject, which can be further modified by the user.Example:
There is a test that shows that cards with tables and plots are already possible to be parsed, reproducing the original content 1:1, with the exception of line breaks. The latter should generally be conserved, but there are a few cases where there is one too many, but IMO it doesn't matter.
Comment
In the end, this turned out easier than I initially thought it would. The main difficulty are the data structures returned by the pandoc parser, for which I couldn't find any documentation. I guess Haskell code is just so self-documenting it's not needed.
For this reason (and out of laziness), there are probably quite a few edge cases that I haven't covered yet. Just as an example, when parsing tables, pandoc tells us how the columns are aligned. This information is currently completely discarded (we let tabulate choose the alignment). If we want to preserve the table alignment, we would need to make some changes.
I think, however, that it's fine not to support everything out of the box. We can incrementally add support for the rest as we collect more experience.
Implementation
This feature requires the alternative card implementation from #203
pandoc is used for the following reasons:
The disadvantage is that pandoc is not a Python package, so users need to install it separately. But it is available on all common platforms.
For calling pandoc, I chose to shell out using
subprocess. I think this should be fine but LMK if there is a better way.There is a Python package that binds pandoc (https://github.com/boisgera/pandoc) but I don't think it's worth it for us to add it, just to avoid shelling out. The package seems to have low adoption and contains a bunch of stuff we don't need.
I chose to implement this feature such that the parser that generates the
Cardobject should not have to know anything about markdown. Everything related to markdown is moved to a separate class in_markup.pywhich could be extended in the future to support other markup languages.In an ideal world, we would not have to know anything about markdown at all. Instead the
Cardobject should have methods (similar to what we already have foradd_tableetc.) that handles all special cases in a markup-language agnostic way. But in practice, this is far from being true. E.g. if a user wants to add bold text, there is no special method for it, so they would need to add raw markdown. TheCardclass is thus a leaky abstraction, which requires the parsing to directly deal with markdown.TODOs
This PR is not finished. Remaining TODOs that come to mind:
Documentation has to be updated in several placesCI needs to install pandoc so that the tests are actually runpandoc is installed in CI for the ubuntu built, not windows or mac -- IMO that's enough.There are some specifics here that won't work with all Python versions, like the use ofTypedDictClean up a couple of TODO comments, docstrings, and assertscard.loadmethod that uses pandoc parsing?