Skip to content

Fix num. of minimal calls to the Hub with peft for pipeline - #26385

Merged
ydshieh merged 7 commits into
mainfrom
fix_pip_min_call
Oct 13, 2023
Merged

Fix num. of minimal calls to the Hub with peft for pipeline #26385
ydshieh merged 7 commits into
mainfrom
fix_pip_min_call

Conversation

@ydshieh

@ydshieh ydshieh commented Sep 25, 2023

Copy link
Copy Markdown
Collaborator

What does this PR do?

This is a continuation of #25715 where @sgugger fix test_cached_model_has_minimum_calls_to_head but not test_cached_pipeline_has_minimum_calls_to_head (as I didn't mention this to him).

This PR mostly copies the logic from #25715.

The current failing test and the error are

tests/pipelines/test_pipelines_common.py::CustomPipelineTest::test_cached_pipeline_has_minimum_calls_to_head
(line 905)  AssertionError: 2 != 1

Comment on lines +737 to +741
pretrained_model_name_or_path = None
if isinstance(config, str):
pretrained_model_name_or_path = config
elif config is None and isinstance(model, str):
pretrained_model_name_or_path = model

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

try to get the repo. in different case before going to the next block of getting config, similar to the block in line 758

f"{model} is not a valid model_id."
)
task = get_task(model, use_auth_token)
task = get_task(model, token)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

use_auth_token is already deprecated.

@ydshieh
ydshieh requested a review from LysandreJik September 25, 2023 14:13
@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Sep 25, 2023

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

@LysandreJik LysandreJik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems okay to me generally, would like another review as it's touching critical code.

Not too sure how we could do it, but it would also be nice to have tests for this, as it's the kind of thing that will continue to break unless we actively test it.

Comment thread src/transformers/pipelines/__init__.py Outdated
Comment thread src/transformers/pipelines/__init__.py
@ydshieh

ydshieh commented Sep 26, 2023

Copy link
Copy Markdown
Collaborator Author

This seems okay to me generally, would like another review as it's touching critical code.

Not too sure how we could do it, but it would also be nice to have tests for this, as it's the kind of thing that will continue to break unless we actively test it.

We do have tests: 4 tests named test_cached_xxx_has_minimum_calls_to_head. This is why I found the failure to fix.

elif config is None and isinstance(model, str):
# Check for an adapter file in the model path if PEFT is available
if is_peft_available():
subfolder = hub_kwargs.get("subfolder", None)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

subfolder was never in hub_kwargs here

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me, thanks for expanding my fix!

@ydshieh

ydshieh commented Oct 13, 2023

Copy link
Copy Markdown
Collaborator Author

Merge now.

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Feel free to merge!

@ydshieh
ydshieh merged commit 288bf5c into main Oct 13, 2023
@ydshieh
ydshieh deleted the fix_pip_min_call branch October 13, 2023 09:03
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.

5 participants