Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Make the merged checksum file available to the installer #3597

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Feb 6, 2025

resolves #3575 (BA-635)

The checksum files have been consolidated into one.
The installer changes from downloading the checksum files for each package separately to receiving a consolidated checksum file and using them separately.

Checklist: (if applicable)

  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options

@Yaminyam Yaminyam requested a review from achimnol February 6, 2025 07:36
@github-actions github-actions bot added the size:M 30~100 LoC label Feb 6, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/ai/backend/install/context.py:1025

  • [nitpick] The error message could be more informative by including the actual URL of the checksum file.
raise ValueError(f"Checksum for {pkg_name} not found in {csum_path}")

Comment on lines +1009 to 1011
async with self.wget_sema:
await wget(csum_url, dst_path, progress)

Copy link
Preview

Copilot AI Feb 6, 2025

Choose a reason for hiding this comment

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

The download of the checksum file should handle potential errors to avoid issues if the download fails.

Suggested change
async with self.wget_sema:
await wget(csum_url, dst_path, progress)
try:
async with self.wget_sema:
await wget(csum_url, dst_path, progress)
except (aiohttp.ClientError, asyncio.TimeoutError) as e:
self.log.write(f"Failed to download {csum_url}: {e}")
raise RuntimeError(f"Failed to download checksum file from {csum_url}. Please check your network connection and try again.")

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@HyeockJinKim HyeockJinKim added this pull request to the merge queue Feb 7, 2025
Merged via the queue into main with commit 9fbcb67 Feb 7, 2025
20 checks passed
@HyeockJinKim HyeockJinKim deleted the fix/install-merged-checksum branch February 7, 2025 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M 30~100 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The checksum file is integrated and the installer is not working
2 participants