FIX Fix line breaks in markdown tables #156

BenjaminBossan · 2022-09-28T16:00:17Z

Solves #154

Description

Markdown table entires shouldn't contain "\n" line breaks. Instead, those should be replaced by "<br />". This is now done for both the hyperparameter table and for tables added through card.add_table.

An example of a fixed hyperparameter table: https://huggingface.co/skops-ci/hf_hub_example-a4b4abaf-d1c3-4300-a5ac-cc9e3c65bd39#hyperparameters

Implementation

The bugfix has to be applied after calling tabulate, as the table entries can contain arbitrary objects, whose repr is only called by tabulate. That also means we have to be careful as the tabulated table string will contain valid "\n" line breaks that should be left untouched.

Unfortunately, the hyperparameter table doesn't use card.add_table, which is why the fix has to be applied in two places. IMO it would be better if it used that method under the hood but that would result in the hyperparameter table being placed in a different section (because of the inflexibility of the templating).

A minor naming issue I found: The method that actually creates the hyperparameter table is called _extract_estimator_config. I would have never guessed to be honest.

Markdown tables shouldn't contain "\n" line breaks. Instead, they should be replaced by "<br />". This is now done for both the hyperparameter table and for tables added through card.add_table. Implementation The bugfix has to be applied after calling tabulate, as the table entries can contain arbitrary objects, whose repr is only called by tabulate. That also means we have to be careful as the tabulated table string will contain valid "\n" line breaks that should be left untouched. Unfortunately, the hyperparameter table doesn't use card.add_table, which is why the fix has to be applied in two places. IMO it would be better if it used the same method under the hood but that would result in the hyperparameter table being placed in a different section (because of the inflexibility of the templating). A minor naming issue I found: The method _extract_estimator_config actually creates the hyperparameter table. I would have never guessed to be honest.

BenjaminBossan · 2022-09-28T16:07:15Z

Ready for review @skops-dev/maintainers

Note: CI is still failing because of the breaking change in huggingface_hub.

merveenoyan · 2022-09-28T16:49:06Z

@BenjaminBossan Can we just pin the version of HF hub used in CI for "test" separately until we get up to date with Hub changes?

skops/skops/_min_dependencies.py

Line 14 in 132925b

"huggingface_hub": ("0.9.0rc3", "install", None),

merveenoyan

LGTM, thanks a lot! can you please post the model cards (of the skorch example and another example that table already works) after this change here?

BenjaminBossan · 2022-09-28T17:06:42Z

Can we just pin the version of HF hub used in CI for "test" separately until we get up to date with Hub changes?

I suggested pinning as a quick fix until we get a proper fix here.

can you please post the model cards

I linked this above, that should be sufficient, right?

adrinjalali · 2022-09-29T10:38:22Z

docs/changes.rst

@@ -16,6 +16,8 @@ v0.3
 - Add ``private`` as an optional argument to :meth:`.hub_utils.push` to
  optionally set the visibility status of a repo when pushing to the hub.
  :pr:`130` by `Adrin Jalali`_.
+- Fix a bug that resulted in markdown tables being rendered incorrectly if
+  entries contained line breaks by `Benjamin Bossan`_.


PR number missing here.

adrinjalali · 2022-09-29T10:39:27Z

skops/card/_model_card.py

+    # replace line breaks "\n" with html tag <br />, however, leave end-of-line
+    # line breaks (eol_lb) intact
+    eol_lb = "|\n"
+    placeholder = "$%!?"  # arbitrary sting that never appears naturally


as a friend once said, equivalent to banging my head against the keyboard lol

It's stupid :D If you have a better idea, please let me know

About the PR number: I fixed that. How do you add the correct number before you even created the PR? Do you just check the highest number so far and hope that until you actually open the PR, there hasn't been another PR in-between?

I don't, I just add that as a commit after I create the PR :D

…an/skops into bugfix-line-break-in-tables

Ignore if there are multiple whitespaces in tables by replacing all n>1 whitespaces with n=1 whitespace.

Also ignore multiple "-", as in the 2nd line of a markdown table.

BenjaminBossan · 2022-10-04T11:56:20Z

@skops-dev/maintainers I fixed the failing CI tests, so the PR is ready for re-review.

The error was that the markdown table had different padding between the expected and the actual outcome, e.g.:

expected:

| foo | bar |
|-----|-----|
|  1  |  2  |

result:

| foo |  bar  |
|-----|-------|
|  1  |   2   |

Neither Adrin nor I could reproduce the error locally, even on the same OS and Python version. As this padding is only relevant when reading the table in raw text but not for how it's rendered, I opted to remove it completely when comparing the resulting table with the expected table:

after processing:

| foo | bar |
|-|-|
| 1 | 2 |

I'm not super happy with this but it seemed like an okay solution. Alternatively, I thought about parsing the markdown table into a pandas DataFrame, but that seemed to be less rigorous. If you know any better solution, LMK.

BenjaminBossan requested review from adrinjalali and removed request for adrinjalali September 28, 2022 16:07

merveenoyan approved these changes Sep 28, 2022

View reviewed changes

Merge branch 'main' into bugfix-line-break-in-tables

d1112d4

adrinjalali reviewed Sep 29, 2022

View reviewed changes

BenjaminBossan added 5 commits September 29, 2022 17:14

Merge branch 'main' into bugfix-line-break-in-tables

dc78f1e

Merge branch 'bugfix-line-break-in-tables' of github.com:BenjaminBoss…

8f6726d

…an/skops into bugfix-line-break-in-tables

Try to fix broken tests

1d5fb40

Ignore if there are multiple whitespaces in tables by replacing all n>1 whitespaces with n=1 whitespace.

Try to fix broken tests

4ba9f5a

Also ignore multiple "-", as in the 2nd line of a markdown table.

Try to fix broken tests

f79c636

Also ignore multiple "-", as in the 2nd line of a markdown table.

adrinjalali approved these changes Oct 4, 2022

View reviewed changes

adrinjalali changed the title ~~BUG Fix line breaks in markdown tables~~ FIX Fix line breaks in markdown tables Oct 4, 2022

adrinjalali merged commit f41f891 into skops-dev:main Oct 4, 2022

BenjaminBossan deleted the bugfix-line-break-in-tables branch October 5, 2022 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Fix line breaks in markdown tables #156

FIX Fix line breaks in markdown tables #156

BenjaminBossan commented Sep 28, 2022

BenjaminBossan commented Sep 28, 2022 •

edited

Loading

merveenoyan commented Sep 28, 2022

merveenoyan left a comment •

edited

Loading

BenjaminBossan commented Sep 28, 2022

adrinjalali Sep 29, 2022

adrinjalali Sep 29, 2022

BenjaminBossan Sep 29, 2022

adrinjalali Sep 30, 2022

BenjaminBossan commented Oct 4, 2022

FIX Fix line breaks in markdown tables #156

FIX Fix line breaks in markdown tables #156

Conversation

BenjaminBossan commented Sep 28, 2022

Description

Implementation

BenjaminBossan commented Sep 28, 2022 • edited Loading

merveenoyan commented Sep 28, 2022

merveenoyan left a comment • edited Loading

Choose a reason for hiding this comment

BenjaminBossan commented Sep 28, 2022

adrinjalali Sep 29, 2022

Choose a reason for hiding this comment

adrinjalali Sep 29, 2022

Choose a reason for hiding this comment

BenjaminBossan Sep 29, 2022

Choose a reason for hiding this comment

adrinjalali Sep 30, 2022

Choose a reason for hiding this comment

BenjaminBossan commented Oct 4, 2022

BenjaminBossan commented Sep 28, 2022 •

edited

Loading

merveenoyan left a comment •

edited

Loading