Download from HuggingFace with private token #390

frostedoyster · 2024-11-13T14:40:45Z

As in the title

Contributor (creator of pull-request) checklist

Tests updated (for new features and bugfixes)?
Documentation updated (for new features)?
Issue referenced (for PRs that solve an issue)?

📚 Documentation preview 📚: https://metatrain--390.org.readthedocs.build/en/390/

abmazitov · 2024-11-13T22:04:05Z

I managed to adapt the PET.load_checkpoint() method, so now it works with the HuggingFace checkpoint that we have. However the mtt export API is still not well-adapted. I keep getting the following error:

Traceback (most recent call last):
  File "/home/mazitov/apps/anaconda3/envs/metatrain/lib/python3.11/site-packages/metatrain/__main__.py", line 105, in main
    export_model(**args.__dict__)
TypeError: export_model() got an unexpected keyword argument 'huggingface_api_token'

which is related to the fact that in the load_model function, we pop the 'huggingface_api_token' key from kwargs, which is the copy of the args.__dict__, so the original dict still has the unexpected key.

Even though loading the model explicitly worked for me, it only worked while loading from the abmazitov/pet-mad and not from the metatensor/pet-mad. Maybe I did something wrong...

docs/src/getting-started/checkpoints.rst

src/metatrain/utils/io.py

Luthaf · 2024-11-14T11:33:59Z

src/metatrain/utils/io.py

+        # make sure to copy the checkpoint to the current directory
+        shutil.copy(path, Path.cwd() / str(path).split("/")[-1])


I dont understand why this is required?

And if you want the file to be directly downloaded to the current directory you can do

path, _ = urlretrieve( url=str(path), filename=str(Path.cwd() / str(path).split("/")[-1]))

See https://docs.python.org/3/library/urllib.request.html#urllib.request.urlretrieve.

This is required for fine-tuning. mtt export only makes the .pt available in the current directory, but that's useless for fine-tuning

Can you change this to use filename in urlretrieve, and add the same INFO message about adding a file to cwd?

Luthaf · 2024-11-14T11:35:41Z

I like this! Down the line, we could maybe add a way to read the API token from env variables or a file; and use such mechanism for all external integrations (W&B seems like a very logical one to add next).

PicoCentauri · 2024-11-14T16:18:05Z

If you have a private model on Hugging Face, there are effective third-party tools like wget, curl, or the Hugging Face web interface to download it. Adding a private API interface introduces overhead and complexity to the codebase, and for a private Hugging Face repo, this doesn’t provide significant benefits, as there are accessible alternatives. The presented workaround also introduces an additional dependency, and I’m concerned it will require maintenance soon. We’re moving around URL schemas, and, to my knowledge, Hugging Face doesn’t have a stable schema, right?

As for W&B, it’s a different story, and I fully support adding a functionality, because users can’t access the benefits of W&B without us integrating it directly into the code.

Luthaf · 2024-11-15T14:45:37Z

If you have a private model on Hugging Face, there are effective third-party tools like wget, curl, or the Hugging Face web interface to download it.

I think having it enabled inside mtt export makes sense: we already download the models ourself in all other cases, no need for an external tool.

One point one which I am not very clear though is can one actually download a private HF model without a token? I.e. just wget https://https://huggingface.co/........./? If so, then I agree there is no need for this code to be added. But I would be very surprised if one can download a private model without some form of authentication.

We’re moving around URL schemas, and, to my knowledge, Hugging Face doesn’t have a stable schema, right?

This is a good point, maybe a better interface here would look like mtt export --mode huggingface --hf-api-token=.... --hf-owner=who/what --hf-file=file-path (very bad syntax, just for the idea. This could also be huggingface://who/what/file-path or huggingface://token@who/what/file-path or whatever).

The presented workaround also introduces an additional dependency, and I’m concerned it will require maintenance soon.

I'm not very concerned by this: one would hope that HF has enough workforce to keep this working without breaking changes, given their size and that this Python module is part of their core offering. It also only touch a relatively small part of our code and is fairly well isolated. But we should add a test on CI to catch any breaking change anyway!

PicoCentauri · 2024-11-19T06:26:00Z

One point one which I am not very clear though is can one actually download a private HF model without a token? I.e. just wget https://https://huggingface.co/........./? If so, then I agree there is no need for this code to be added. But I would be very surprised if one can download a private model without some form of authentication.

You might be right that this doesn't work. Would be quite some risk if just having the link is enough to fetch a model.

I'm not very concerned by this: one would hope that HF has enough workforce to keep this working without breaking changes, given their size and that this Python module is part of their core offering. It also only touch a relatively small part of our code and is fairly well isolated. But we should add a test on CI to catch any breaking change anyway!

Okay we can see how this evolves. If the maintenance work is too high we can also easily remove the code again. But, yes a test in CI is the minimum we should do.

frostedoyster · 2024-11-25T11:23:14Z

Just for completeness: yes, public HF models should be available with wget without going through the new HF functionality here, which is just there for private models

Luthaf · 2024-11-25T12:41:29Z

src/metatrain/utils/io.py

+        # get repo_id and filename
+        split_path = path.split("/")
+        repo_id = f"{split_path[3]}/{split_path[4]}"  # org/repo
+        filename = ""


Instead of doing string manipulation, should we pass more structured information to this code? Copied from my comment on the overall PR:

maybe a better interface here would look like mtt export --mode huggingface --hf-api-token=.... --hf-owner=who/what --hf-file=file-path (very bad syntax, just for the idea. This could also be huggingface://who/what/file-path or huggingface://token@who/what/file-path or whatever).

I.e. instead of trying to guess the info we need, ask the use about it

I don't think so because HuggingFace gives you the download link, so that's what most users will use

Asking for the "split information" on the command line is just more work for the user

Luthaf · 2024-11-25T12:42:34Z

src/metatrain/utils/io.py

+            filename = filename[10:]
+        path = hf_hub_download(repo_id, filename, token=kwargs["huggingface_api_token"])
+        # make sure to copy the checkpoint to the current directory
+        shutil.copy(path, Path.cwd() / str(path).split("/")[-1])


Should this be added to the log? Some kind of INFO: downloaded $FILE_NAME from hugingface.co/owner/repo to current directory

Luthaf · 2024-11-25T12:43:11Z

src/metatrain/utils/io.py

+        # make sure to copy the checkpoint to the current directory
+        shutil.copy(path, Path.cwd() / str(path).split("/")[-1])


Can you change this to use filename in urlretrieve, and add the same INFO message about adding a file to cwd?

Luthaf · 2024-11-25T12:46:34Z

tests/cli/test_export_model.py

@@ -108,3 +110,41 @@ def test_reexport(monkeypatch, tmp_path):
    export_model(model_loaded, "exported_new.pt")

    assert Path("exported_new.pt").is_file()
+
+
+def test_private_huggingface(monkeypatch, tmp_path):


This will fail for people running the tests locally, unless they have access to the repo & a token set, right?

Could we skip the test if HUGGINGFACE_TOKEN_METATRAIN is unset? And set it on CI/locally as needed. (using a different name in case people are already using HUGGINGFACE_TOKEN for some other reason.

Luthaf · 2024-11-25T14:08:52Z

src/metatrain/utils/io.py

+        shutil.copy(path, Path.cwd() / filename)
+        logger.info(f"Downloaded model from HuggingFace to {filename}")


If I understand the code properly, this will download repo/owner/.../main/some/path/to/file.ckpt to <cwd>/some/path/to/file.ckpt. Is this intended? Otherwise we should be using os.path.basename(filename) as the destination

Luthaf · 2024-11-25T14:09:58Z

src/metatrain/utils/io.py

        shutil.copy(path, Path.cwd() / str(path).split("/")[-1])
+        logger.info(f"Downloaded model to {str(path).split('/')[-1]}")


Suggested change

shutil.copy(path, Path.cwd() / str(path).split("/")[-1])

logger.info(f"Downloaded model to {str(path).split('/')[-1]}")

basename = os.path.basename(str(path))

shutil.copy(path, Path.cwd() / basename)

logger.info(f"Downloaded model to ./{basename}")

should be clearer and also work on windows where path are \ separated.

Luthaf · 2024-11-25T14:10:44Z

@frostedoyster could you NOT merge without approval …

frostedoyster and others added 4 commits November 13, 2024 15:40

Download from HuggingFace with private token

4bda3db

Remove print

e2aa785

Fix HuggingFace download

7de66e9

Allow direct load of the checkpoint from the Hugging Face

b69db89

frostedoyster added 3 commits November 14, 2024 07:41

Hack it so it works

ce06288

More elegant solution

8cf49c4

Documentation

918efa1

frostedoyster marked this pull request as ready for review November 14, 2024 10:31

frostedoyster requested a review from abmazitov as a code owner November 14, 2024 10:31

Make sure remote checkpoints are also saved to the current directory

a18fd37

Luthaf reviewed Nov 14, 2024

View reviewed changes

frostedoyster and others added 2 commits November 17, 2024 08:56

Update downloader to accept multiple HF URL formats

1ea8605

Merge branch 'main' into huggingface-download

632e76f

frostedoyster added 2 commits November 25, 2024 12:23

Improve docs

7dc0859

Add test

8d132e0

frostedoyster force-pushed the huggingface-download branch from fe7e06e to 8d132e0 Compare November 25, 2024 12:11

frostedoyster requested review from PicoCentauri and Luthaf November 25, 2024 12:15

Luthaf reviewed Nov 25, 2024

View reviewed changes

frostedoyster added 4 commits November 25, 2024 14:26

Log, change env variable name, skip test, use filename variable

7266d4b

Merge branch 'main' into huggingface-download

77a53e3

Change logging

a2a7e3f

Fix escape

8440d47

frostedoyster merged commit 1ffccb4 into main Nov 25, 2024
13 checks passed

frostedoyster deleted the huggingface-download branch November 25, 2024 14:04

Luthaf reviewed Nov 25, 2024

View reviewed changes

frostedoyster mentioned this pull request Nov 25, 2024

Fix code in remote IO #404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download from HuggingFace with private token #390

Download from HuggingFace with private token #390

frostedoyster commented Nov 13, 2024 •

edited

Loading

abmazitov commented Nov 13, 2024

Luthaf Nov 14, 2024

PicoCentauri Nov 14, 2024

frostedoyster Nov 25, 2024

Luthaf Nov 25, 2024

Luthaf commented Nov 14, 2024

PicoCentauri commented Nov 14, 2024

Luthaf commented Nov 15, 2024

PicoCentauri commented Nov 19, 2024

frostedoyster commented Nov 25, 2024

Luthaf Nov 25, 2024

frostedoyster Nov 25, 2024

frostedoyster Nov 25, 2024

Luthaf Nov 25, 2024

frostedoyster Nov 25, 2024

Luthaf Nov 25, 2024

Luthaf Nov 25, 2024

Luthaf Nov 25, 2024

Luthaf Nov 25, 2024

Luthaf commented Nov 25, 2024

		# make sure to copy the checkpoint to the current directory
		shutil.copy(path, Path.cwd() / str(path).split("/")[-1])

		shutil.copy(path, Path.cwd() / filename)
		logger.info(f"Downloaded model from HuggingFace to {filename}")

		shutil.copy(path, Path.cwd() / str(path).split("/")[-1])
		logger.info(f"Downloaded model to {str(path).split('/')[-1]}")

-        shutil.copy(path, Path.cwd() / str(path).split("/")[-1])
-        logger.info(f"Downloaded model to {str(path).split('/')[-1]}")
+        basename = os.path.basename(str(path))
+        shutil.copy(path, Path.cwd() / basename)
+        logger.info(f"Downloaded model to ./{basename}")

Download from HuggingFace with private token #390

Download from HuggingFace with private token #390

Conversation

frostedoyster commented Nov 13, 2024 • edited Loading

Contributor (creator of pull-request) checklist

abmazitov commented Nov 13, 2024

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

Luthaf commented Nov 14, 2024

PicoCentauri commented Nov 14, 2024

Luthaf commented Nov 15, 2024

PicoCentauri commented Nov 19, 2024

frostedoyster commented Nov 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf commented Nov 25, 2024

frostedoyster commented Nov 13, 2024 •

edited

Loading