Skip to content

Conversation

@daavoo
Copy link
Contributor

@daavoo daavoo commented Jul 7, 2022

Cutting quite a few corners to cover the minimum for treeverse/dvclive#91

Proper refactor would make sense as follow-up because the whole package was focused on a single output format (HTML).


table: Add generate_markdown.

image: Add generate_markdown.

vega: Add generate_markdown.

"Translate" vega plot to matplotlib figure to save as png.

Introduce render_markdown.

Closes #62

@daavoo daavoo requested a review from pared July 7, 2022 10:45
def render_html(
renderers: List["Renderer"],
output_file: "StrPath",
metrics: Optional[Dict[str, Dict]] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover from #61

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2022

Codecov Report

❌ Patch coverage is 89.87342% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.73%. Comparing base (2d5cc29) to head (a50c863).
⚠️ Report is 126 commits behind head on main.

Files with missing lines Patch % Lines
src/dvc_render/markdown.py 60.60% 13 Missing ⚠️
src/dvc_render/vega.py 93.10% 2 Missing ⚠️
src/dvc_render/table.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
- Coverage   97.18%   95.73%   -1.45%     
==========================================
  Files          16       19       +3     
  Lines         497      633     +136     
  Branches       79       92      +13     
==========================================
+ Hits          483      606     +123     
- Misses          8       22      +14     
+ Partials        6        5       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should require users to install altair to support all vega plots in md?

@daavoo
Copy link
Contributor Author

daavoo commented Jul 8, 2022

Maybe we should require users to install altair to support all vega plots in md?

I initially considered it but decided to go with matplotlib mainly because:

  • matplotlib covers the DVCLive<>CML use case (in the future we need to revisit this and consider altair for cover all use cases)

  • even though altair and matplotlib are similarly heavier, matplotlib is much popular among ML and as such more likely to be already installed in the user env. For altair it feels like we could be the only reason to install a heavy dependency in the env

  • In order to save images, altair requires additional dependency altair_saver which in turn requires a even heavier non-python dependency:

    To be used, it requires the Selenium Python package, and a properly configured installation of either chromedriver or geckodriver.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now wonder if this use case is not another point for dvc-render to have some support for javascript. If there is some pythonic wrapper for js maybe we could leverage vega to produce images in non-linear template use cases. But I guess that would require some research to determine this idea's feasibility.

image: Add `generate_markdown`.

vega: Add `generate_markdown`.

"Translate" vega plot to `matplotlib` figure to save as `png`.

Introduce `render_markdown`.

Closes #62
@daavoo daavoo merged commit 546a50d into main Jul 11, 2022
@daavoo daavoo deleted the markdown branch July 11, 2022 13:55
daavoo added a commit that referenced this pull request Jul 26, 2022
DVC passes some values as `rich.Text` instead of string.
As an oversight in #69 , the refactor to use `list_dict_to_dict_list` removed this string casting, breaking `dvc exp show --pcp`
daavoo added a commit that referenced this pull request Jul 26, 2022
DVC passes some values as `rich.Text` instead of string.
As an oversight in #69 , the refactor to use `list_dict_to_dict_list` removed this string casting, breaking `dvc exp show --pcp`
daavoo added a commit that referenced this pull request Jul 26, 2022
DVC passes some values as `rich.Text` instead of string.
As an oversight in #69 , the refactor to use `list_dict_to_dict_list` removed this string casting, breaking `dvc exp show --pcp`
daavoo added a commit that referenced this pull request Jul 26, 2022
DVC passes some values as `rich.Text` instead of string.
As an oversight in #69 , the refactor to use `list_dict_to_dict_list` removed this string casting, breaking `dvc exp show --pcp`
daavoo added a commit that referenced this pull request Jul 26, 2022
DVC passes some values as `rich.Text` instead of string.
As an oversight in #69 , the refactor to use `list_dict_to_dict_list` removed this string casting, breaking `dvc exp show --pcp`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support markdown as output format

3 participants