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

Updates to py_require() #1718

Merged
merged 12 commits into from
Jan 27, 2025
Merged

Updates to py_require() #1718

merged 12 commits into from
Jan 27, 2025

Conversation

t-kalinowski
Copy link
Member

@t-kalinowski t-kalinowski commented Jan 25, 2025

Changes:

  • Update order of discovery:

    • continue to discover r-reticualte venv before bootstrapping
    • only bootstrap a venv if initializing python
    • give a way to opt out
  • Update uv_get_or_create_env():

    • pass through stderr from uv
    • simplify pre+post processing
  • Update uv_binary()

    • allow resolving without bootstraping install
  • Update uv cache resolution:

    • Fix issue where reticulate resolved the incorrect cache dir if a user-managed uv installation is used. Right now this is done by forcing to always use the reticulate cache, but it might make sense instead to let uv use it's default uv cache dir if the uv installation is not managed by reticulate.
  • Changes to tests:

    • declare requirements for tests with py_require()
    • capture stderr from uv in tests
    • use color in snapshots
    • skip tests around cache resolution that fail if using user-installed uv
    • skip tests of py_require() that fail if python is already initialized

@t-kalinowski
Copy link
Member Author

Restarting R session...

> reticulate::repl_python()
Python 3.13.1 (/Users/tomasz/Library/Caches/r-reticulate/uv-cache/archive-v0/Kc-xLPZ1uy6xA8d7Kourf/bin/python3)
Reticulate 1.40.0.9000 REPL -- A Python interpreter in R.
Enter 'exit' or 'quit' to exit the REPL and return to R.
>>> 
  • We might want to change how we present the path to the python installation here, seeing the actual path in the cache dir isn't helpful. Maybe something like:

    Python 3.13.1 (Reticulate managed installation)
    Installed Python Packages: numpy, pandas, ...<other requirements from py_require()>
    Reticulate 1.40.0.9000 REPL -- A Python interpreter in R.
    Enter 'exit' or 'quit' to exit the REPL and return to R.
  • We should probably default to Python 3.11 here, given this is a fresh R session.

@edgararuiz
Copy link
Collaborator

We should probably default to Python 3.11 here, given this is a fresh R session.

You're talking about only for repl_python(), right?

@edgararuiz
Copy link
Collaborator

We might want to change how we present the path to the python installation here, seeing the actual path in the cache dir isn't helpful.

I would suggest to go even simpler:

Python version: 3.13.1 
Installed Python packages: numpy, pandas, ...<other requirements from py_require()>
Reticulate 1.40.0.9000 REPL -- A Python interpreter in R.
Enter 'exit' or 'quit' to exit the REPL and return to R.

@t-kalinowski
Copy link
Member Author

You're talking about only for repl_python(), right?

Good question. Maybe we should modify the print methods for py_config and py_last_error too, to avoid showing the path to the cached env.

@edgararuiz
Copy link
Collaborator

Actually, using UV_PYTHON seems to be a good mechanism to set a "soft" default:

> x <- uv_get_or_create_env()
> system2(x, c("--version"))     
Python 3.13.1
> 
> withr::with_envvar(
+   list("UV_PYTHON" = "3.11"), {
+     x <- uv_get_or_create_env()
+     system2(x, c("--version"))
+   }
+ )
Python 3.11.11                   
> 
> withr::with_envvar(
+   list("UV_PYTHON" = "3.11"), {
+     x <- uv_get_or_create_env(python_version = c("3.10"))  
+     system2(x, c("--version"))
+   }
+ )
Python 3.10.16                   
> 
> withr::with_envvar(
+   list("UV_PYTHON" = "3.11"), {
+     x <- uv_get_or_create_env(python_version = c(">=3.11", "<=3.12"))  
+     system2(x, c("--version"))
+   }
+ )
Python 3.12.0  

@t-kalinowski t-kalinowski marked this pull request as ready for review January 27, 2025 21:06
@t-kalinowski t-kalinowski merged commit 4a50238 into main Jan 27, 2025
16 checks passed
@t-kalinowski
Copy link
Member Author

Thanks Edgar! I merged to unblock Max; We'll address remaining issues in followup PR's.

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.

2 participants