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

chore: Change arrow scalar ids usage #4347

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

camenares
Copy link
Contributor

[PENDING] 2. Run unit tests and ensure that they are passing: https://github.com/feast-dev/feast/blob/master/CONTRIBUTING.md#unit-tests

What this PR does / why we need it:

Changes where _ARROW_SCALAR_IDS_TO_BQ is imported, so that google-cloud-bigquery can be bumped. This PR only bumps to 3.13.0, but unblocks further bumping. _ARROW_SCALAR_IDS_TO_BQ was moved here.

Which issue(s) this PR fixes:

Related to #3823, but provides a different solution than downgrading.

@tokoko
Copy link
Collaborator

tokoko commented Jul 13, 2024

This will break all versions below 3.14, won't it? Maybe we should just copy the mapping over instead... Not ideal, but might be better than to depend on an internal helper function. @sudohainguyen wdyt?

@sudohainguyen
Copy link
Collaborator

good fix!
@camenares could you help re-generate the requirements txt files?
to make sure all other packages are compatible 😄

@sudohainguyen
Copy link
Collaborator

please also check that the change is backward compatible for those who using version less that 3.12

@camenares
Copy link
Contributor Author

@tokoko that's a good point, I think you're right, it would not work 3.12 (but will double check).

I think there's three options:

  1. Change setup.py to also strict pin to 3.13.0
  2. Do a try/catch and try to import from each helper
  3. Copy the mapping over.

I will work on regenerating the requirements.txt's

@tokoko
Copy link
Collaborator

tokoko commented Jul 15, 2024

@camenares option 2 also sounds good to me. We can resort to copying the mapping if some other issue arises in the future. Any reason for the upper bound set at <3.14. Why can't we change to <4? Are there some other known issues?

P.S. You need to sign all your commits (git commit -s). Follow the instructions here to fix the DCO check.

@camenares camenares force-pushed the change-arrow-scalar-ids-usage branch from 53ca7f1 to 3ab01e0 Compare July 15, 2024 19:39
Signed-off-by: Christopher Camenares <ccamenares@gmail.com>
@camenares camenares force-pushed the change-arrow-scalar-ids-usage branch from 5f34a59 to 7cab31b Compare July 16, 2024 17:52
Signed-off-by: Christopher Camenares <ccamenares@gmail.com>
@tokoko
Copy link
Collaborator

tokoko commented Jul 16, 2024

Can you run make lint-python and make format-python commands?

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>
@camenares
Copy link
Contributor Author

@tokoko yep! Fixing that now, I believe it's related to python/mypy#1153 but testing this locally

@camenares
Copy link
Contributor Author

@tokoko I also noticed that allowing the bound to be <4 failed the full integration tests (local were fine). Restricting to <3.14.0 seems to let those pass though.

@tokoko
Copy link
Collaborator

tokoko commented Jul 16, 2024

@camenares integration tests failure was unrelated, sorry about that. We accidentally merged a failing PR to master.

@camenares
Copy link
Contributor Author

@tokoko Oh! Thanks for that, I just figured it was worth trying <3.14. Let me bump to <4.0 again then.

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>
@tokoko
Copy link
Collaborator

tokoko commented Jul 16, 2024

You will have to regenerate requirements files for the bump to take effect in ci.

@camenares
Copy link
Contributor Author

@tokoko <4.0 worked! I see no additional changes after running make lock-python-dependencies-all.

Copy link
Collaborator

@tokoko tokoko left a comment

Choose a reason for hiding this comment

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

right, looks like firebase-admin lib is pinning with an upper bound, I'll look into that later. lgtm otherwise, thanks a lot

@tokoko tokoko merged commit 5a16364 into feast-dev:master Jul 16, 2024
15 checks passed
@camenares
Copy link
Contributor Author

camenares commented Jul 17, 2024

Thanks a lot for the guidance @tokoko , looking forward to more Feast contributions in the future!

EDIT: I see integration-tests-and-build / integration-test-python (3.10, ubuntu-latest) failed on the commit, but it looks like that (or the test for another python version) regularly fails on some commits.

@camenares camenares deleted the change-arrow-scalar-ids-usage branch July 17, 2024 00:46
@tokoko
Copy link
Collaborator

tokoko commented Jul 17, 2024

we have some strict rate limits on cloud services that our tests are often hitting, you can ignore it.

nick-amaya-sp pushed a commit to sailpoint/feast that referenced this pull request Jul 23, 2024
* Update google-cloud-storage

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>

* test tighter library restriction

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>

* fix lint

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>

* bump <4 again

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>

---------

Signed-off-by: Christopher Camenares <ccamenares@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants