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

image: resolve abolute path from src #39

Merged
merged 1 commit into from May 26, 2022

Conversation

pared
Copy link
Contributor

@pared pared commented Apr 29, 2022

Related to iterative/dvc#7664

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #39 (0d27d74) into main (28593d2) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   95.68%   95.80%   +0.11%     
==========================================
  Files          14       14              
  Lines         464      477      +13     
  Branches       76       77       +1     
==========================================
+ Hits          444      457      +13     
  Misses         15       15              
  Partials        5        5              
Impacted Files Coverage Δ
src/dvc_render/base.py 100.00% <100.00%> (ø)
src/dvc_render/html.py 70.00% <100.00%> (ø)
src/dvc_render/image.py 100.00% <100.00%> (ø)
src/dvc_render/plotly.py 100.00% <100.00%> (ø)
src/dvc_render/vega.py 100.00% <100.00%> (ø)
tests/test_image.py 100.00% <100.00%> (ø)

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 28593d2...0d27d74. Read the comment docs.

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

@pared sorry for my lack of understanding but I'm not sure I follow.

The HTML generated by this P.R. iterative/dvc#7664 looks fine even without this P.R?

HTML there contains absolute paths to the image so when I open the HTML in the browser it renders correctly?

@pared
Copy link
Contributor Author

pared commented May 2, 2022

@daavoo Yes, it will work.
Hovewer, if we want to make HTML reports portable (eg copy dir and send via email) we need working relative paths. In iterative/dvc#7549 I made plots use relative paths, but relative to cwd and not index.html.

@@ -20,17 +22,26 @@ class ImageRenderer(Renderer):

EXTENSIONS = {".jpg", ".jpeg", ".gif", ".png"}

def partial_html(self) -> str:
def partial_html(self, output_dir=None, **kwargs) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this arg? Afaik if outs was passed on DVC side, it should be present in self.properties:

https://github.com/iterative/dvc/blob/d0e05c4b66dc6e8b5e4ee46b622d42ab8b3fb01d/dvc/render/match.py#L48-L49

It makes more ense here if we expect this to be used outside DVC 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking a bit about it, i think that this was a proper approach. out contains information where to write the images. So in a sense it contains the same information as datapoints.src just without exact name of the file. When generating HTML via render_html, we can provide index.html file path, and it is the one path we are interested in the most. To be sure that our relpath is correct, we need to evaluate index.html path and calculate relative path to its dirname. Renamed output_dir to more informative html_path.

@pared pared merged commit 869459d into iterative:main May 26, 2022
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.

None yet

3 participants