Skip to content
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

Add markdown support. #69

Merged
merged 1 commit into from
Jul 11, 2022
Merged

Add markdown support. #69

merged 1 commit into from
Jul 11, 2022

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jul 7, 2022

Cutting quite a few corners to cover the minimum for iterative/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
@@ -81,7 +81,6 @@ def embed(self) -> str:
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

Merging #69 (8bce146) into main (70e2da2) will decrease coverage by 1.48%.
The diff coverage is 89.54%.

@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
- Coverage   97.14%   95.65%   -1.49%     
==========================================
  Files          16       19       +3     
  Lines         491      622     +131     
  Branches       77       90      +13     
==========================================
+ Hits          477      595     +118     
- Misses          8       22      +14     
+ Partials        6        5       -1     
Impacted Files Coverage Δ
src/dvc_render/markdown.py 60.60% <60.60%> (ø)
src/dvc_render/table.py 81.81% <90.90%> (+8.48%) ⬆️
src/dvc_render/vega.py 97.18% <92.59%> (-2.82%) ⬇️
src/dvc_render/base.py 100.00% <100.00%> (ø)
src/dvc_render/exceptions.py 100.00% <100.00%> (ø)
src/dvc_render/html.py 87.50% <100.00%> (+3.50%) ⬆️
src/dvc_render/image.py 100.00% <100.00%> (ø)
src/dvc_render/plotly.py 100.00% <100.00%> (ø)
src/dvc_render/utils.py 100.00% <100.00%> (ø)
tests/test_image.py 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e2da2...8bce146. Read the comment docs.

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
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support markdown as output format
3 participants