-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(python): Relative path resolution for plugin libraries #21911
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21911 +/- ##
==========================================
- Coverage 80.48% 80.48% -0.01%
==========================================
Files 1636 1636
Lines 236700 236709 +9
Branches 2693 2693
==========================================
+ Hits 190504 190507 +3
- Misses 45563 45568 +5
- Partials 633 634 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think this still assumes your virtual env is in your current working directory, which is not always the case |
Thanks for pointing that out. Probably the correct approach should to be to find the |
23697d4
to
f8deb29
Compare
Someone can still change their |
d9e27bf
to
0b9e4e0
Compare
What do you mean by change of cwd? |
I think in the current state the following code won't work: import os
import polars as pl
import polars_hash as plh
ldf = pl.LazyFrame({"a": ["1", "2", "3"]}).with_columns(h=plh.col("a").chash.sha2_256())
os.chdir("..")
df = ldf.collect() because the relative path calculation happens when the I propose to save the plugin's path relative to the venv when it's created (as in your previous PR), but to change the code that actually loads the plugin (in |
Ah okay I see what you mean now. I did not consider that directory could change before collecting the |
- Uses `os.path.relpath` since it is less strict about diverging paths - Simplifies testing of plugin path resolution
bbb96fe
to
002d2d6
Compare
we just tested this solution and it seems to work well. I was able to move graphs from one .venv and run them in another .venv. Do you think this could get some attention and perhaps get finally merged ? @ritchie46 |
Alright. Thanks all! |
Following up on #21675.
The relative path plugin cannot be resolved because the root
.venv
is not included in the calculated path. This fixes it by calculating the path relative to the parent ofsys.prefix