skops icon indicating copy to clipboard operation
skops copied to clipboard

Utility function to add arbitrary files to the repo

Open BenjaminBossan opened this issue 3 years ago • 3 comments

Solves #111

This is the outcome of using plot_hf_hub.py with one added line:

hub_utils.add_files([__file__], dst=local_repo)

As can be seen, the script itself is now included in the upload.

BenjaminBossan avatar Aug 31 '22 15:08 BenjaminBossan

@skops-dev/maintainers

Could you help me figure out why the new tests are failing? The offending code seems to stem from this fixutre:

https://github.com/BenjaminBossan/skops/blob/adding-files/skops/hub_utils/tests/test_hf_hub.py#L497-L510

which results in the error OSError: Model file '/tmp/.../model.pickle' does not exist.

This seems odd to me because the tests pass locally and because I'm doing basically exactly the same as, e.g., this test, which succeeds.

BenjaminBossan avatar Aug 31 '22 15:08 BenjaminBossan

@skops-dev/maintainers The PR is ready for review now.

The error mentioned above was caused by test_inference doing some cleanup that would disrupt tests running later. We use some session-scoped fixtures, resulting in this dependency. My solution was to have test_inference run in a different directory in order for it not to interfere with other tests. Alternatively, we could reduce the scope of the used fixtures, but that would slow the tests down.

Regarding the failed codecov check: I have covered all new lines with tests, so this is a false positive. It seems that codecov thinks that my changes affect the coverage of get_model_output (see very last line):

https://app.codecov.io/gh/skops-dev/skops/compare/123/tree/skops/hub_utils/_hf_hub.py#D1L634

This is probably caused by this CI run not randomly failing when calling inference API, so it's a fluke.

BenjaminBossan avatar Sep 01 '22 11:09 BenjaminBossan

@adrinjalali Yes, I was thinking about making files *args too, I changed it as suggested. Furthermore, I added an exist_ok argument (default True). If the file exists and exist_ok=True, I'll now overwrite it (instead of skipping like previously) and if exist_ok=False raise an error.

I was also thinking about adding directories but if we want that, let's do it in another PR.

BenjaminBossan avatar Sep 01 '22 14:09 BenjaminBossan