Skip to content

Conversation

@MonkeyCanCode
Copy link
Contributor

@MonkeyCanCode MonkeyCanCode commented Jan 14, 2025

When running tests locally for more than once, we will get a error message for wrong version of platformdirs (it will get auto resolved as pip will try to update it). This is due to the version of poetry we are using. Here is the same error messages:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
tox 4.23.2 requires platformdirs>=4.3.6, but you have platformdirs 3.11.0 which is incompatible.
Successfully installed build-0.10.0 cachecontrol-0.12.14 keyring-23.13.1 platformdirs-3.11.0 poetry-1.5.0 poetry-core-1.6.0 poetry-plugin-export-1.5.0 xattr-0.10.1

[notice] A new release of pip is available: 24.2 -> 24.3.1
[notice] To update, run: pip install --upgrade pip
Installing dependencies from lock file

Package operations: 0 installs, 1 update, 0 removals

  • Updating platformdirs (3.11.0 -> 4.3.6)

This PR update the version of poetry from 1.5.0 to 1.8.5 (latest 1.x version released at Dec 2024...the next version is 2.x).

@snazy
Copy link
Member

snazy commented Jan 14, 2025

I'm not a Python guru, but why not go straight to Poetry 2.0.1?

@snazy
Copy link
Member

snazy commented Jan 14, 2025

Other than that I think that all versions should be managed by Renovate and not manually hidden in scripts or text or Dockerfiles. A "dummy" requirements.txt should can help there.

@MonkeyCanCode
Copy link
Contributor Author

Other than that I think that all versions should be managed by Renovate and not manually hidden in scripts or text or Dockerfiles. A "dummy" requirements.txt should can help there.

Thanks @snazy for the quick review. Usually 2.X to 1.X will bring major breaking changes and those are released within last 1 month as well. But if there is no concern, I can look into this and see if we can use the latest version. Also, I do agreed using fixed version in multiple script files is bad idea. Let me refactor this via a requirements.txt instead.

@MonkeyCanCode MonkeyCanCode marked this pull request as draft January 14, 2025 16:00
@snazy
Copy link
Member

snazy commented Jan 15, 2025

Usually 2.X to 1.X will bring major breaking changes and those are released within last 1 month as well.

I'm fine with either. I couldn't find any information about how long 1.x will be supported though (the 2.0.0 announcement post and the docs say nothing 🤷 ). Having said that, maybe it's more "future proof" to to to 2.0.1?

@MonkeyCanCode
Copy link
Contributor Author

Usually 2.X to 1.X will bring major breaking changes and those are released within last 1 month as well.

I'm fine with either. I couldn't find any information about how long 1.x will be supported though (the 2.0.0 announcement post and the docs say nothing 🤷 ). Having said that, maybe it's more "future proof" to to to 2.0.1?

So I did some checks tonight on this, their latest stable version is still 1.8.5 and 2.x is on their main version. I have the changes ready for my local setup. But it seems like the Quarkus merge broke a couple things there. @adutra already created bug trackers for those. I will raise the PR again once I am able to verify the local changes.

@MonkeyCanCode MonkeyCanCode marked this pull request as ready for review January 17, 2025 06:10
@MonkeyCanCode
Copy link
Contributor Author

@snazy this is ready for review again...had to make good amount of manual changes on local for running the needed end to end test...most of the needed steps are covered in #804 and it is pending for https://github.com/apache/polaris/pull/610...once both are in, I can do another review to fill-in the missing pieces if any.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

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

Ah! Nice and simple :)
The requirements.txt approach should work.

Should poetry used in /polaris and /getting-started/spark/notebooks/Dockerfile be bumped as well? For the former, there's also #756, if that helps. For the latter I don't mind that much, because it's rather a demo that doesn't have any CI coverage, so bumping things there could break it.

@MonkeyCanCode
Copy link
Contributor Author

Ah! Nice and simple :) The requirements.txt approach should work.

Should poetry used in /polaris and /getting-started/spark/notebooks/Dockerfile be bumped as well? For the former, there's also #756, if that helps. For the latter I don't mind that much, because it's rather a demo that doesn't have any CI coverage, so bumping things there could break it.

that is fair point. Yeah, I saw #756 and is thinking about to take it. I was waiting for those Quarkus fixes to be merge then start doing more validations and check-in more code. I seems to have a working setup from couple PR from @adutra that are not yet merged. So yeah, no longer a blocker for me to take more tasks. Again, thanks for the quick review.

@snazy
Copy link
Member

snazy commented Jan 24, 2025

@MonkeyCanCode don't wanna merge this?

@MonkeyCanCode
Copy link
Contributor Author

@MonkeyCanCode don't wanna merge this?

No concern from me to merge this one. However, I don't have access to do so. @flyrain can you help here if this changes looks good to you?

@adutra
Copy link
Contributor

adutra commented Jan 24, 2025

@MonkeyCanCode please reach out to @jbonofre to fix your permissions. You should be able to merge since you are a committer.

@adutra
Copy link
Contributor

adutra commented Jan 24, 2025

I can merge now if you want, or we wait for @flyrain – your call.

@MonkeyCanCode
Copy link
Contributor Author

I can merge now if you want, or we wait for @flyrain – your call.

Go for it. Thanks so much. I will reach out to JB to get access later.

@adutra adutra merged commit 1db6120 into apache:main Jan 24, 2025
5 checks passed
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.

3 participants