Skip to content

fix(hf_push): dispatch dataset vs model after the permission prompt - #16

Open
arthi-arumugam-git wants to merge 1 commit into
Liquid4All:mainfrom
arthi-arumugam-git:fix-hf-push-permission-import
Open

fix(hf_push): dispatch dataset vs model after the permission prompt#16
arthi-arumugam-git wants to merge 1 commit into
Liquid4All:mainfrom
arthi-arumugam-git:fix-hf-push-permission-import

Conversation

@arthi-arumugam-git

Copy link
Copy Markdown

Fixes #15.

agent.py imported _execute_hf_push, which commit 1594e14 ("auto-detect dataset vs model") split into _execute_hf_push_dataset and _execute_hf_push_model. The call site was missed, so approving an hf_push permission prompt raised ImportError before anything reached the Hub.

$ python -c "from lqh.tools.handlers import _execute_hf_push"
ImportError: cannot import name '_execute_hf_push' from 'lqh.tools.handlers'

handle_hf_push returns PERMISSION_REQUIRED on the first push to a given repo, so this is the path every new user takes.

Why it is more than a rename

@marekolszewski flagged in the issue that the two replacements take different arguments. They do, and there were two further problems on that path:

It assumed dataset. The old code globbed for parquet unconditionally and indexed parquet_files[0]. A model folder has no parquet, so a plain rename to _execute_hf_push_dataset would have turned the ImportError into an IndexError for model pushes.

It treated parquet_files as paths. _detect_hf_repo_type returns file names; handle_hf_push builds the path as target / parquet_files[0]. The old code built its own list of full paths, so the two representations differed.

The change

The call site now mirrors the dispatch at the end of handle_hf_push: it honours an explicit repo_type from the tool call, falls back to _detect_hf_repo_type, and calls whichever executor applies with that function's own argument order. Nothing new is invented; it is the working path's logic, reused.

If detection comes back None it returns a validation failure rather than guessing. handle_hf_push rejects that case before ever prompting, so reaching it here means the folder changed between the prompt and the answer.

Tests

Two, both failing on main at agent.py:2190:

  • a dataset folder reaching _execute_hf_push_dataset with data.parquet resolved correctly
  • a model folder containing only config.json reaching _execute_hf_push_model, which is the case a rename would have left broken
tests/unit: 90 failed, 1881 passed, 17 skipped   (this branch)
tests/unit: 90 failed, 1879 passed, 17 skipped   (main)

Identical failure count; the 90 are pre-existing TUI keyboard tests, unrelated to this path. Passed goes up by exactly the two tests added here.

One thing worth your call

The root cause is that these two paths duplicate each other, and 1594e14 updated only one of them. This PR keeps them separate and makes them agree, which is the smaller change. The alternative is to have _handle_hf_push_permission re-invoke handle_hf_push once permission is granted, so there is only ever one dispatch and this cannot recur.

I went with mirroring because it is the minimal fix for a filed bug. Happy to do the collapse instead if you would prefer it.

agent.py imported _execute_hf_push, which commit 1594e14 split into
_execute_hf_push_dataset and _execute_hf_push_model. The permission path is
the first push to any new repo, so approving the prompt raised ImportError
before anything reached the Hub. Fixes Liquid4All#15.

The two replacements take different arguments, so this is more than a rename.
The call site now mirrors the dispatch at the end of handle_hf_push: it uses
_detect_hf_repo_type, honours an explicit repo_type from the tool call, and
falls back to detection.

That also fixes a second problem on the same path. It previously assumed
dataset, globbed for parquet and indexed parquet_files[0], which raises
IndexError on a model folder, so a plain rename would have left model pushes
broken. It also treated parquet_files as paths, where handle_hf_push treats
them as names relative to target.

Adds two tests, both failing on main: a dataset folder reaching
_execute_hf_push_dataset, and a model folder with only config.json reaching
_execute_hf_push_model.
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.

hf_push fails with ImportError after the user approves it

1 participant