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

Add support for setting up Python through renv #448

Open
wants to merge 81 commits into
base: main
Choose a base branch
from

Conversation

milanmlft
Copy link
Contributor

Hi sandpaper devs!

I have been experimenting with ways to set up Python and manage Python dependencies in sandpaper lessons through renv::use_python(). This is mainly geared towards running Python code chunks in RMarkdown files.

To see this in action, I set up a toy sandpaper lesson here: https://github.com/milanmlft/toy-sandpaper-python
Note that I also had to update the carpentries GitHub Actions slightly to get this to work: https://github.com/milanmlft/actions. If relevant, I can create a PR for that as well.

I thought this might be useful for other sandpaper users and would love to get your feedback!

This would also address #61 (I think).

@zkamvar zkamvar added the enhancement New feature or request label May 3, 2023
@zkamvar
Copy link
Contributor

zkamvar commented May 3, 2023

@milanmlft, THANK YOU SO MUCH FOR THIS PR!

To manage expectations: I (at the moment, the sole maintainer) am quite overencumbered right now and will not be able to review this until mid-June 😞

That being said, I can see this being a real boon to some of our python lessons! I'm really happy that you've written the tests, functionality, and created a working demo 🤩.

One comment I can make right now WRT to the actions: in carpentries/actions@3532acf, instead of doing this, you can set sandpaper: milanmlft/sandpaper@use_python in your lesson config.yaml

Additionally, I think this is something @sabaferdous12 might be interested in.

@zkamvar
Copy link
Contributor

zkamvar commented May 3, 2023

/document

@milanmlft
Copy link
Contributor Author

Hi @zkamvar, thanks for the nice feedback! And no worries about the review. For my own purposes, I can continue with my own fork for now. If anything else pops up, I'll add it here as well.

One problem I currently still have is that update_cache() does not work nicely with the Python dependencies. When requirements.txt contains some Python packages, but those are not actually installed, then update_cache() will remove those packages from requirements.txt, because renv::update() does not update Python dependencies and so when renv::snapshot() is called, it doesn't detect those packages and thus removes them from the requirements.txt. manage_deps() used to do this as well, but there I found a workaround, implemented in cea48bf.

Another note is the bug reported in rstudio/renv#1217, for which I implemented a workaround in 1495f89.

Oh and coincidentally, I'm actually working together with @sabaferdous12 on a project, which is exactly why I implemented this 😅

@zkamvar
Copy link
Contributor

zkamvar commented May 18, 2023

There are some test failures here, but that's because they need to be skipped on Windows by the fact that the earlier context skips them on Windows.

I'm still working on the documentation for contributors, but see here about tests: #455 (comment)

I believe these are skipped on Windows because of the idiosyncracies with using {renv} on the windows runner (though I know it works well on Windows)

@milanmlft
Copy link
Contributor Author

Alright, I'll adapt the tests accordingly. I'm still doing some work on the tests anyway because I'm not entirely happy with them yet. I'll push some more updates in the coming days!

@zkamvar
Copy link
Contributor

zkamvar commented May 19, 2023

Alright, I'll adapt the tests accordingly. I'm still doing some work on the tests anyway because I'm not entirely happy with them yet. I'll push some more updates in the coming days!

Awesome!

Also FYI (and for @sabaferdous12), #465 modifies the render_html() function, which I believe was modified in the L2D fork of sandpaper, so be aware of that potential conflict when updating that fork.

[pull] main from carpentries:main
[pull] main from carpentries:main
[pull] main from carpentries:main
[pull] main from carpentries:main
Correctly check for lines in `requirements.txt`
@milanmlft
Copy link
Contributor Author

Pinging the current sandpaper maintainers @froggleston and @ErinBecker. I know that adding new features is not a priority right now, but would still kindly ask if you could have a look at this :)
I think it would benefit lesson developers who want to create lessons with Python, and we've actually been using a version of this successfully in a few Carpentries-style lessons:

If there's anything I can do to speed up the process, please let me know!
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants