Parser that allows to load dumped model cards

[BenjaminBossan]

As discussed here:

skops-dev#72 (comment)

@E-Aho @merveenoyan @adrinjalali I created this PR against my alternative-card-implementation PR since it build on it, thus pinging you here. Please let me know what you think of this approach and if we should continue with this feature.

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.

Example:

numpy as np
sklearn.linear_model import LinearRegression
skops.card import Card

X = np.array([[1, 1], [1, 2], [2, 2], [2, 3]])
y = np.dot(X, np.array([1, 2])) + 3
regr = LinearRegression().fit(X, y)
card = Card(regr)
card.save("README.md")

# later, load the card again
skops.card import parse_card

parsed_card = parse_modelcard("README.md")

# continue editing the card
parsed_card.add(**{"My new section": "My new content"})

# overwrite old card with new one
parsed_card.save("README.md")

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 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 feature 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 which 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 Card object should have methods (similar to what we already have for add_table etc.) 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. The Card class 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:

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 instance 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
6. Clean up a couple of TODO comments and asserts

[BenjaminBossan]

I added support for the meta info yaml section of the start of model cards, as well as a bunch more markdown syntax. We now have:

• Space
• Strong
• Emph
• Strikeout
• Subscript
• Superscript
• Plain
• Str
• RawInline
• RawBlock
• SoftBreak
• LineBreak
• Para
• Header
• Image
• CodeBlock
• Code
• Table
• Div
• Link
• BulletList
• Quoted

The tests are extended to include the model card of the most popular model, bert-base-uncased. Probably there should be tests for the model cards of the top 5 models or so.

The test almost passes, there is still a diff for table alignment, trailing whitespace, some line breaks and indentation, but all the important content is replicated. Here is the complete diff:

< | Model | #params | Language |
< |------------------------|--------------------------------|-------|
< | [`bert-base-uncased`](https://huggingface.co/bert-base-uncased) | 110M   | English |
< | [`bert-large-uncased`](https://huggingface.co/bert-large-uncased)              | 340M    | English | sub
< | [`bert-base-cased`](https://huggingface.co/bert-base-cased)        | 110M    | English |
< | [`bert-large-cased`](https://huggingface.co/bert-large-cased) | 340M    |  English |
< | [`bert-base-chinese`](https://huggingface.co/bert-base-chinese) | 110M    | Chinese |
< | [`bert-base-multilingual-cased`](https://huggingface.co/bert-base-multilingual-cased) | 110M | Multiple |
< | [`bert-large-uncased-whole-word-masking`](https://huggingface.co/bert-large-uncased-whole-word-masking) | 340M | English |
< | [`bert-large-cased-whole-word-masking`](https://huggingface.co/bert-large-cased-whole-word-masking) | 340M | English |
---
> | Model                                                                                                   | #params   | Language   |
> |---------------------------------------------------------------------------------------------------------|-----------|------------|
> | [`bert-base-uncased`](https://huggingface.co/bert-base-uncased)                                         | 110M      | English    |
> | [`bert-large-uncased`](https://huggingface.co/bert-large-uncased)                                       | 340M      | English    |
> | [`bert-base-cased`](https://huggingface.co/bert-base-cased)                                             | 110M      | English    |
> | [`bert-large-cased`](https://huggingface.co/bert-large-cased)                                           | 340M      | English    |
> | [`bert-base-chinese`](https://huggingface.co/bert-base-chinese)                                         | 110M      | Chinese    |
> | [`bert-base-multilingual-cased`](https://huggingface.co/bert-base-multilingual-cased)                   | 110M      | Multiple   |
> | [`bert-large-uncased-whole-word-masking`](https://huggingface.co/bert-large-uncased-whole-word-masking) | 340M      | English    |
> | [`bert-large-cased-whole-word-masking`](https://huggingface.co/bert-large-cased-whole-word-masking)     | 340M      | English    |
65c65
< You can use the raw model for either masked language modeling or next sentence prediction, but it's mostly intended to
---
> You can use the raw model for either masked language modeling or next sentence prediction, but it’s mostly intended to
197c197
< the other cases, it's another random sentence in the corpus. Note that what is considered a sentence here is a
---
> the other cases, it’s another random sentence in the corpus. Note that what is considered a sentence here is a
211c211
< used is Adam with a learning rate of 1e-4, \\(\beta_{1} = 0.9\\) and \\(\beta_{2} = 0.999\\), a weight decay of 0.01,
---
> used is Adam with a learning rate of 1e-4, \(\beta_{1} = 0.9\) and \(\beta_{2} = 0.999\), a weight decay of 0.01,
220,223c220,222
< | Task | MNLI-(m/mm) | QQP  | QNLI | SST-2 | CoLA | STS-B | MRPC | RTE  | Average |
< |:----:|:-----------:|:----:|:----:|:-----:|:----:|:-----:|:----:|:----:|:-------:|
< |      | 84.6/83.4   | 71.2 | 90.5 | 93.5  | 52.1 | 85.8  | 88.9 | 66.4 | 79.6    |
< 
---
> | Task   | MNLI-(m/mm)   |   QQP |   QNLI |   SST-2 |   CoLA |   STS-B |   MRPC |   RTE |   Average |
> |--------|---------------|-------|--------|---------|--------|---------|--------|-------|-----------|
> |        | 84.6/83.4     |  71.2 |   90.5 |    93.5 |   52.1 |    85.8 |   88.9 |  66.4 |      79.6 |
248c247
< 	<img width="300px" src="https://cdn-media.huggingface.co/exbert/button.png">
---
> <img width="300px" src="https://cdn-media.huggingface.co/exbert/button.png">

The table alignment might not be possible to replicate exactly with tabulate.

IMO, this is already quite a good replication but it's hard to write a test that would allow this kind of diff to pass.

[adrinjalali]

looks pretty descent already!

[adrinjalali]

then doesn't it make sense for them to be _md_image/etc instead?

[BenjaminBossan]

Yes, I'll change that, wasn't sure how necessary that would be since it's in a private module. I'll also remove the md_ prefix, since the class already indicates that it's markdown.

[adrinjalali]

IMO, this is already quite a good replication but it's hard to write a test that would allow this kind of diff to pass.

So blue is what we generate? It looks better to me anyway. I'm happy with the results.

[BenjaminBossan]

So blue is what we generate? It looks better to me anyway. I'm happy with the results.

Green is what we generate, red is the original. Probably you mean it looks better because the width of the columns is aligned, which is true. Of course once rendered as html it doesn't matter, but when looking at the raw markdown, it looks indeed better.

[adrinjalali]

I'm colorblind, so for me GH shows green as blue and red as yellow-ish (I think).

Cool, then we're good. Let me know when you need a better review of this PR.

[BenjaminBossan]

Cool, then we're good. Let me know when you need a better review of this PR.

I'll try to finish the alternative model card PR today, once that's done I'll make this PR ready.

[BenjaminBossan]

Closing in favor of skops-dev#257