ENH Copy plots on Card save #330

lazarust · 2023-03-28T02:10:51Z

Reference Issues/PRs

Fixes #300

What does this implement/fix? Explain your changes.

Implements copying files to the path of the repository.

Any other comments?

lazarust · 2023-03-28T02:13:46Z

@BenjaminBossan @adrinjalali Here's the draft PR, there's not much because I wanted to make sure I was thinking of this correctly.

From what I understood from the thread on the issue, we're wanting to copy the files to the repo path whenever add_plot method is called. Is that understanding correct?

Also, I've been trying to think of how best to test this. Do y'all have any ideas?

Sorry, this PR is so small. I just wanted to make sure I was heading in the right direction before jumping into the deep end.

adrinjalali

The copying should probably happen when the user calls save on a Card object. And instead of path_in_repo, we should probably call it sth like relative_path or something, and then copy all plots when save is called, to a path relative to where the path to save is given. WDYT?

adrinjalali · 2023-03-28T14:09:14Z

skops/card/_model_card.py

+            if path_in_repo is not None:
+                shutil.copy(plot_path, path_in_repo)


this should be in the above section, and modify the plot_path variable accordingly.

So just to clarify, we shouldn't be doing this copy when add_plot is called. It should only be done in Card.save(). Is that right?

yes 👍🏼

@adrinjalali Hmm, after thinking through this some I'm unsure if copying on model card save is the best. If we do it when a user calls add_plot() then we'd already have the path of the plot in the context. From what I understand of how .save() is working we'd have to loop through all the generated content, find the plots, and then save them.

That feels a little extra to me since we could just save the copy when a user adds the plot. What are your thoughts? Am I misunderstanding how .save() is working?

You're right that save would do that in an iteration. But from the user's experience, to me it makes more sense. This way, users only provide a relative path to the destination of the model card, and at the end we save them all. It should also provide a more consistent experience since they can't mess up addresses and at the end not have them in the repo.

I agree with Adrin that copying on save has some advantages that make it worth the extra effort. On top of what was said, it would help to avoid the problem of a user adding a plot, then overwriting the plot file, but that change not being reflected in the final model card.

Implementation-wise, I agree with Thomas that looping through all sections again is a bit wasteful. However, I think this feature can be implemented without a second loop. For that, I would add an argument to _generate_card to optionally copy the figures. This argument is passed down to _generate_content, which can take care of the copying. This way, we don't iterate through all sections twice and avoid duplicating the logic (which can be problematic besides performance, e.g. the current implementation does not consider invisible sections).

Another implementation detail, instead of if hasattr(section, "path"), I would have probably done if isinstance(section, PlotSection), but I'm open to be convinced otherwise :)

This reverts commit 010a305.

adrinjalali · 2023-04-05T09:57:48Z

I'd be interested in what @BenjaminBossan thinks here too though.

lazarust · 2023-04-07T00:29:06Z

@BenjaminBossan I've moved the logic into _generate_content and this is ready to review! Thanks! 👍🏽

BenjaminBossan

Thanks for doing the adjustments.

I wonder if we can simplify the implementation a bit. Right now, the code considers the possibility that plot_path != destination_path but I wonder if that's relevant. If I store my model card into foo/bar/, would I ever want my plots to be copied to spam/eggs/? I can't think of a use case, but maybe I miss something?

If this is indeed not required, the implementation could be simplified by only having one new argument to _generate_content, which is the destination path. The path of the figure is already known from plot_section.path, so everything required for copying would be there.

From the user point of view, I'd also be unsure what the point of the plot_path argument is in Card.save, I think it needs a bit more explanation. Personally, I would also change the argument to be something like copy_files: bool. If True, it copies all the required files (which, at this point, are only the plots, but who knows what the future brings) to the same directory as the README.md. WDYT?

PS: It would be nice to extend the tests so that we have 2 plots, one with a relative path (as is right now) and one with an absolute path, to ensure that both work.

skops/card/tests/test_card.py

skops/card/_model_card.py

adrinjalali · 2023-04-13T08:46:56Z

I can't think of a use case, but maybe I miss something?

+1

lazarust · 2023-04-16T01:31:00Z

@BenjaminBossan Thanks for the feedback! I refactored this to copy the files into the same path as the README. I also went ahead and added a second plot to the test.

(Side note it looks like there are 34 warnings when running pytest. Is that something that needs fixing? If so I can start working on a PR)

BenjaminBossan

Thanks for implementing the suggested changes. I think there is still some work to do on the tests and the implementation could be simplified. Please take a look.

skops/card/tests/test_card.py

skops/card/_model_card.py

BenjaminBossan · 2023-04-20T09:22:44Z

Sorry, I accidentally submitted "Approve" when I wanted to submit "Request changes", I don't know how to change it now -__-

Still need to get the copying working with absolute paths.

BenjaminBossan · 2023-04-28T10:01:38Z

@lazarust Thanks for working on the test, but it looks like something is broken now. Could you please check?

lazarust · 2023-05-03T00:13:45Z

@BenjaminBossan Should be working now!

BenjaminBossan · 2023-05-03T09:46:48Z

Thanks for fixing the test. The one failing CI job is unrelated, so please ignore it.

From the comments of my last review, not everything was addressed, right? Will you do that?

lazarust · 2023-05-05T00:54:20Z

@BenjaminBossan Sorry about that, I've removed the copy_files parameter from everywhere other than Card.save()

BenjaminBossan

Great, thanks for working in my suggestion and merging main to fix the CI issue. I found a few very minor things to improve, but after this it's good to merge.

BenjaminBossan · 2023-05-05T09:52:43Z

skops/card/_model_card.py

@@ -1410,13 +1425,18 @@ def save(self, path: str | Path) -> None:
        path: str, or Path
            Filepath to save your card.

+        plot_path: str
+            Filepath to save the plots.


Could you please elaborate on this? E.g. mention under what circumstances users may want to use this parameter.

BenjaminBossan · 2023-05-05T09:54:38Z

skops/card/_model_card.py

        Notes
        -----
        The keys in model card metadata can be seen `here
        <https://huggingface.co/docs/hub/models-cards#model-card-metadata>`__.
        """
        with open(path, "w", encoding="utf-8") as f:
-            f.write("\n".join(self._generate_card()))
+            if not isinstance(path, Path):


There is a bit of redundancy here: You cast path to a Path object here but _generate_card and _generate_content both accept str and the latter does Path(destination_path). So either you just pass path without conversion, or those methods should not accept str and don't perform a 2nd round of conversion.

BenjaminBossan · 2023-05-05T09:56:13Z

skops/card/_model_card.py

-            f.write("\n".join(self._generate_card()))
+            if not isinstance(path, Path):
+                path = Path(path)
+            f.write("\n".join(self._generate_card(path.parent if copy_files else None)))


Just a nit, but this is more readable IMO:

Suggested change

f.write("\n".join(self._generate_card(path.parent if copy_files else None)))

destination_path = path.parent if copy_files else None

f.write("\n".join(self._generate_card(destination_path=destination_path)))

lazarust · 2023-05-17T00:32:49Z

@BenjaminBossan Sorry this took so long for me to get back to! This should be ready for you to take another look!

BenjaminBossan

Thanks a lot, we're always happy to have your contributions.

WIP copy files to path_in_repo

010a305

adrinjalali reviewed Mar 28, 2023

View reviewed changes

lazarust and others added 5 commits March 28, 2023 19:47

Merge branch 'main' into ENH-copy-files-for-model-card

94fda14

Revert "WIP copy files to path_in_repo"

83c041a

This reverts commit 010a305.

Moves copying plots to Card.save()

1476294

Merge branch 'main' into ENH-copy-files-for-model-card

8439620

Adds test

ba77226

lazarust changed the title ~~WIP copy files to path_in_repo~~ ENH Copy plots on Card save Apr 5, 2023

lazarust added 2 commits April 4, 2023 20:54

Makes parameter optional

b4fd730

Updates changelog

92dda94

lazarust and others added 2 commits April 6, 2023 18:55

Merge branch 'main' into ENH-copy-files-for-model-card

b8ca68e

Moves copying to _generate_content

ebe246f

lazarust marked this pull request as ready for review April 7, 2023 00:21

BenjaminBossan requested changes Apr 11, 2023

View reviewed changes

skops/card/tests/test_card.py Outdated Show resolved Hide resolved

skops/card/_model_card.py Outdated Show resolved Hide resolved

skops/card/_model_card.py Outdated Show resolved Hide resolved

lazarust and others added 2 commits April 15, 2023 20:18

Copies plots to the same dir as README

10c92ac

Merge branch 'main' into ENH-copy-files-for-model-card

68f2b5b

lazarust requested a review from BenjaminBossan April 16, 2023 01:29

BenjaminBossan approved these changes Apr 20, 2023

View reviewed changes

skops/card/tests/test_card.py Outdated Show resolved Hide resolved

skops/card/tests/test_card.py Outdated Show resolved Hide resolved

skops/card/tests/test_card.py Outdated Show resolved Hide resolved

skops/card/_model_card.py Outdated Show resolved Hide resolved

lazarust and others added 2 commits April 27, 2023 14:52

Merge branch 'main' into ENH-copy-files-for-model-card

71f8e31

Updated the test based off of feedback

d0778de

Still need to get the copying working with absolute paths.

Fixes test for absolute path

f7f82d0

lazarust and others added 2 commits May 4, 2023 19:38

Merge branch 'main' into ENH-copy-files-for-model-card

b706044

Removes needing to pass copy_files down between methods.

f7c2cf0

lazarust requested a review from BenjaminBossan May 5, 2023 00:53

BenjaminBossan requested changes May 5, 2023

View reviewed changes

lazarust and others added 3 commits May 10, 2023 13:27

Merge branch 'main' into ENH-copy-files-for-model-card

0286b6e

Merge branch 'main' into ENH-copy-files-for-model-card

3e36975

Updates doc string and cleans up parameter types

1872155

lazarust requested a review from BenjaminBossan May 17, 2023 00:32

BenjaminBossan approved these changes May 17, 2023

View reviewed changes

BenjaminBossan merged commit 5d503af into skops-dev:main May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Copy plots on Card save #330

ENH Copy plots on Card save #330

lazarust commented Mar 28, 2023

lazarust commented Mar 28, 2023

adrinjalali left a comment

adrinjalali Mar 28, 2023

lazarust Mar 29, 2023

adrinjalali Mar 30, 2023

lazarust Apr 4, 2023

adrinjalali Apr 4, 2023

BenjaminBossan Apr 5, 2023

adrinjalali commented Apr 5, 2023

lazarust commented Apr 7, 2023

BenjaminBossan left a comment •

edited

Loading

adrinjalali commented Apr 13, 2023

lazarust commented Apr 16, 2023

BenjaminBossan left a comment

BenjaminBossan commented Apr 20, 2023

BenjaminBossan commented Apr 28, 2023

lazarust commented May 3, 2023

BenjaminBossan commented May 3, 2023

lazarust commented May 5, 2023

BenjaminBossan left a comment

BenjaminBossan May 5, 2023

BenjaminBossan May 5, 2023

BenjaminBossan May 5, 2023

lazarust commented May 17, 2023

BenjaminBossan left a comment

		if path_in_repo is not None:
		shutil.copy(plot_path, path_in_repo)

	f.write("\n".join(self._generate_card(path.parent if copy_files else None)))
	destination_path = path.parent if copy_files else None
	f.write("\n".join(self._generate_card(destination_path=destination_path)))

ENH Copy plots on Card save #330

ENH Copy plots on Card save #330

Conversation

lazarust commented Mar 28, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

lazarust commented Mar 28, 2023

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Apr 5, 2023

lazarust commented Apr 7, 2023

BenjaminBossan left a comment • edited Loading

Choose a reason for hiding this comment

adrinjalali commented Apr 13, 2023

lazarust commented Apr 16, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

BenjaminBossan commented Apr 20, 2023

BenjaminBossan commented Apr 28, 2023

lazarust commented May 3, 2023

BenjaminBossan commented May 3, 2023

lazarust commented May 5, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazarust commented May 17, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

BenjaminBossan left a comment •

edited

Loading