Enhance model loading with retries and update .gitignore#49
Enhance model loading with retries and update .gitignore#49suzukimain merged 37 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the model loading mechanism from HuggingFace to use an index-based candidate selection approach with recursive retry logic, replacing the previous loop-based retry system. The changes also add tracking of failed attempts and update the .gitignore to exclude a test virtual environment directory.
Key Changes:
- Replaced loop-based candidate retry logic in
search_huggingface()with an index-based approach using acandidate_indexparameter - Added recursive retry mechanism in three
from_huggingface()methods with_candidate_index,_max_retries, and_failed_counttracking - Removed URL validation via HEAD requests before downloads
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| src/auto_diffusers/pipeline_easy.py | Refactored search_huggingface() to use candidate indexing instead of loops; added recursive retry logic with failure tracking to three from_huggingface() methods across different pipeline classes |
| .gitignore | Added venv_test/ directory to the list of ignored virtual environments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| if candidate["type"] == "diffusers": | ||
| repo_id = candidate["repo_id"] | ||
| hf_repo_info = candidate["repo_info"] | ||
| diffusers_model_exists = True | ||
|
|
||
| if download: | ||
| model_path = DiffusionPipeline.download( | ||
| repo_id, | ||
| token=token, | ||
| cache_dir=cache_dir, | ||
| **kwargs, | ||
| ) | ||
| else: | ||
| model_path = repo_id | ||
|
|
||
| file_name = "" | ||
|
|
||
| elif candidate["type"] == "single_file": | ||
| repo_id = candidate["repo_id"] | ||
| file_name = candidate["file_name"] | ||
| hf_repo_info = candidate["repo_info"] | ||
| diffusers_model_exists = False | ||
|
|
||
| if download: | ||
| model_path = hf_hub_download( | ||
| repo_id=repo_id, | ||
| filename=file_name, | ||
| revision=revision, | ||
| token=token, | ||
| force_download=force_download, | ||
| cache_dir=cache_dir, | ||
| ) | ||
| else: | ||
| model_path = f"https://huggingface.co/{repo_id}/blob/main/{file_name}" | ||
|
|
There was a problem hiding this comment.
The previous implementation included URL validation using HEAD requests before downloading to detect 401 Unauthorized errors early. This validation has been removed in the new implementation. While the new approach delegates error handling to the caller through retries, this means:
- Potentially more expensive failed downloads instead of lightweight HEAD checks
- Less informative error handling - the old code specifically tracked 401 errors and provided targeted messaging about access restrictions
- No early detection of authorization issues
Consider whether the trade-off of removing this validation is acceptable, or if some form of early validation should be retained.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if candidate["type"] == "diffusers": | ||
| repo_id = candidate["repo_id"] | ||
| hf_repo_info = candidate["repo_info"] | ||
| diffusers_model_exists = True |
There was a problem hiding this comment.
Variable diffusers_model_exists is not used.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@suzukimain I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@suzukimain I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
- Created _load_pipeline_with_retry helper function to consolidate retry logic - Updated all three from_huggingface methods to use the shared helper - Reduced code duplication by ~93 lines - Maintained identical behavior across all three pipeline types Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Removed incorrect failed_count increment before warning - Adjusted warning message format to match original code - Ensures backward compatibility with existing behavior Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Restore HEAD validation before model downloads to detect 401 errors early
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "HF_TEST=${{ steps.hf_test.outcome }}" >> $GITHUB_ENV | ||
| echo "CIVITAI_TEST=${{ steps.civitai_test.outcome }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
The environment variables HF_TEST and CIVITAI_TEST set in this step won't be accessible to the copilot-auto-fix job. Environment variables set in one job are not automatically passed to other jobs. Since these variables are being set right before exit 1, they serve no purpose in the current implementation. If you need to pass test results to the next job, consider using job outputs or artifacts instead.
There was a problem hiding this comment.
@copilot Please revise the pull request based on this feedback. this feedback
| logger.info( | ||
| f"checkpoint_path: {hf_checkpoint_status.model_status.download_url}" # type: ignore | ||
| ) |
There was a problem hiding this comment.
The log level for checkpoint_path has been changed from warning to info, which is inconsistent with similar logging statements in other from_huggingface methods (lines 1746, 1988, 2230 still use logger.warning). For consistency across the codebase, consider using logger.warning here as well, or update all similar statements to use the same log level.
There was a problem hiding this comment.
@copilot Please revise the pull request based on this feedback. this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@suzukimain I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@suzukimain I've opened a new pull request, #53, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
…rning when a test fails.
Restore logger.warning for checkpoint_path in HuggingFace loader
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for attempt in range(max_retries + 1): | ||
| try: | ||
| # Search for the model on Hugging Face | ||
| hf_checkpoint_status = search_huggingface( | ||
| pretrained_model_link_or_path, candidate_index=attempt, **kwargs | ||
| ) | ||
|
|
||
| logger.warning( | ||
| f"checkpoint_path: {hf_checkpoint_status.model_status.download_url}" # type: ignore | ||
| ) | ||
| checkpoint_path = hf_checkpoint_status.model_path # type: ignore | ||
|
|
||
| # Check the format of the model checkpoint | ||
| try: | ||
| if hf_checkpoint_status.loading_method == "from_single_file": # type: ignore | ||
| # Load the pipeline from a single file checkpoint | ||
| pipeline = load_pipeline_from_single_file( | ||
| pretrained_model_or_path=checkpoint_path, | ||
| pipeline_mapping=pipeline_mapping, | ||
| **kwargs, | ||
| ) | ||
| else: | ||
| pipeline = cls.from_pretrained(checkpoint_path, **kwargs) | ||
|
|
||
| # Success - show warning if some candidates were skipped | ||
| if failed_count > 0: | ||
| logger.warning( | ||
| f"Note: {failed_count} model(s) skipped due to errors. " | ||
| f"To access gated models, provide a token via the 'token' parameter." | ||
| ) | ||
|
|
||
| return add_methods(pipeline) | ||
|
|
||
| except Exception as e: | ||
| logger.info(f"Failed to load pipeline: {e}") | ||
| failed_count += 1 | ||
| if attempt < max_retries: | ||
| logger.info(f"Trying next candidate (attempt {attempt + 2}/{max_retries + 1})...") | ||
| continue | ||
| else: | ||
| logger.warning( | ||
| f"Note: {failed_count} model(s) skipped due to errors. " | ||
| f"To access gated models, provide a token via the 'token' parameter." | ||
| ) | ||
| raise | ||
|
|
||
| except Exception as e: | ||
| logger.info(f"Candidate {attempt + 1}/{max_retries + 1} search failed: {e}") | ||
| failed_count += 1 | ||
| if attempt < max_retries: | ||
| logger.info(f"Trying next candidate (attempt {attempt + 2}/{max_retries + 1})...") | ||
| continue | ||
| else: | ||
| logger.warning( | ||
| f"Note: {failed_count} model(s) skipped due to errors. " | ||
| f"To access gated models, provide a token via the 'token' parameter." | ||
| ) | ||
| raise |
There was a problem hiding this comment.
The nested try-except blocks in _load_pipeline_with_retries cause incorrect tracking of failed_count. The inner try-except block (lines 1225-1256) increments failed_count when pipeline loading fails, but if it raises an exception, the outer except block (lines 1258-1269) will catch it and increment failed_count again for the same failure. This results in failed_count being incremented twice for a single failed attempt. Consider restructuring to avoid double-counting, or only increment failed_count in one location.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| """ | ||
| Validates a URL is accessible by performing a HEAD request. | ||
| Raises HTTPError if the URL returns 401 Unauthorized. | ||
| Logs and continues for other errors. | ||
|
|
||
| Parameters: | ||
| url (`str`): | ||
| The URL to validate. | ||
| token (`str`, *optional*): | ||
| Authentication token for the request. | ||
| headers (`dict`, *optional*): | ||
| Dictionary of HTTP Headers to send with the request. | ||
| If provided, takes precedence over token. | ||
| timeout (`int`, *optional*, defaults to `2`): | ||
| Timeout in seconds for the HEAD request. | ||
|
|
||
| Raises: | ||
| requests.HTTPError: If the URL returns 401 Unauthorized. | ||
| """ |
There was a problem hiding this comment.
The docstring states that the function "Raises HTTPError if the URL returns 401 Unauthorized" (line 524), but the actual implementation calls response.raise_for_status() on line 555, which raises HTTPError for ALL 4xx and 5xx status codes, not just 401. Update the docstring to accurately reflect that the function raises HTTPError for any 4xx/5xx status code, or modify the implementation to only raise for 401 if that's the intended behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@suzukimain I've opened a new pull request, #54, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@suzukimain I've opened a new pull request, #55, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
Co-authored-by: suzukimain <131413573+suzukimain@users.noreply.github.com>
[WIP] Update model loading enhancements and .gitignore
[WIP] Update model loading retries and adjust .gitignore settings
… issue creation for failures
Improve model loading by adding a candidate index and a retry mechanism. Track and log the count of failed attempts during model loading retries. Update .gitignore to exclude the venv_test directory.