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: pins financial-advisor-spanner itables version #1519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tombotch
Copy link

Description

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Fixes itables not loading properly by pinning the version.

@tombotch tombotch requested a review from a team as a code owner December 12, 2024 09:38
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi @tombotch and reviewers,

I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary to get everyone up to speed:

This PR addresses an issue with the itables library in the finance-advisor-spanner sample application. The problem was that itables wasn't loading correctly. The solution implemented in this PR is to pin the itables version to 2.1.5 in the requirements.txt file. This change ensures that the application uses a compatible version of itables, resolving the loading issue.

Specifically, the only change is a modification to the gemini/sample-apps/finance-advisor-spanner/requirements.txt file. Line 3 was changed from itables to itables == 2.1.5 to specify the exact version.

Debugging's a quest,
Pinning versions, a test,
Code now runs with zest!

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request pins the itables library to version 2.1.5, which is a good practice for reproducibility and avoiding unexpected issues from library updates. However, the provided reasoning for the change "Fixes itables not loading properly" is vague. Providing more context on the specific issue encountered would make the PR description more informative. Additionally, more details on the impact of this change and any potential side effects would be beneficial. Here are a few suggestions to further improve this change.

@@ -1,6 +1,6 @@
streamlit
google-cloud-spanner
itables
itables == 2.1.5

Choose a reason for hiding this comment

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

medium

Consider specifying the version for all other dependencies as well for better reproducibility. This will help ensure that the application behaves consistently across different environments and over time. You can use pip freeze > requirements.txt to generate a list of all dependencies and their versions after setting up your virtual environment.

Suggested change
itables == 2.1.5
itables==2.1.5

@tombotch tombotch force-pushed the tombotch-finvest-itables branch from 8ef4efd to e983fc7 Compare December 12, 2024 11:49
@tombotch tombotch force-pushed the tombotch-finvest-itables branch from e983fc7 to 6ebf179 Compare December 12, 2024 12:00
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.

1 participant