-
Notifications
You must be signed in to change notification settings - Fork 451
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 Python cells #2936
Conversation
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2936 ⚙️ Updating now by workflow run 13391514534. What is Uffizzi? Learn more! |
Wow, this makes me very happy! :) |
|
||
defp eval_pyproject_toml(code, binding, env) do | ||
with :ok <- ensure_pythonx() do | ||
quoted = {{:., [], [{:__aliases__, [alias: false], [:Pythonx]}, :uv_init]}, [], [code]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did we land on the discussion between using the application environment for the install vs an explicit call to uv_init
? If we could simply add it as a dep, it would be better, but I don't recall the details. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think we had to do it after the fact, hence why we pretend this to be its own cell. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the main reason is that to use app config, we would need to change the setup cell code that gets evaluated, and I think that's too much. We should evaluate exactly what the user sees, otherwise errors could be confusing.
We also considered putting the config before the setup cell, but then the config is not a part of Mix.install
cache, which is a problem.
Doing it separately also means it's cached separately from Mix.install
, which is good. The only drawback is that the installation does not end up in priv, but we said we can add extra copy paths to FLAME later.
""" | ||
end | ||
|
||
def language_icon(%{language: "pyproject.toml"} = assigns) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worth keeping in mind that this sends duplicates SVG for each cell. I wonder if instead we should move this to CSS and how we approach other icons?
This is fantastic! 🤩 My only comment is about the UI of the setup cell, I was thinking by having two distinct cells, which are collapsed independently. The addition of Python would work as any other cell, by mousing over:
And then, after configured:
WDYT? |
Co-authored-by: José Valim <[email protected]>
Those are totally separate cells. When I added the cell initially, they were collapsed separately, but to me it felt weird to see only part of the setup at once (and two collapsed items are more noise). One other factor is the "Setup" evaluation button, which I think we should have only one and it effectively evaluates the whole setup section. If any of the setup fails, the runtime need to be restarted to try again. So my idea was to have separate cells, but make the "setup" feel more like a single entity. WDYT? |
What you said makes sense. My only complaint then is that "+ Python" and "↳ Python" feels different. For the plug "+ Python", I guess we could use the regular "+ Python" button that we use to add new cells, perhaps make it always visible. But I am unsure what to do about the "↳ Python". Part of me says it could be removed but given the cell indicator says TOML, I am not sure. In other words, it would be nice if they felt more consistent. |
I started with the regular button, but if the idea is to always show it below the setup cell, then it felt like too much, so I went with a more subtle one. It can also be slightly confusing to use the same + Python buton in two places and it does something very different. |
Thanks to @cocoa-xu and @cigrainger for the ideas, suggestions, and getting this party started! |
This introduces support for evaluating Python code. Once Python is enabled, an extra setup cell appears, where the user can specify the Python version and any packages they need, using the pyproject.toml format. Python cells are interoperable with Elixir cells (in terms of variables), similarly to the Erlang cells we already have.
This works on top of pythonx. In fact, we could make it work as smart cells, however making it first class provides a more consistent experience (e.g. switching cell language, having pyproject.toml in the setup section) and will allow us to implement features beyond the scope of smart cells (e.g. intellisense).
pythoncells.mp4
After these changes, there are a few things I want to revisit regarding code structure, but that can come later, and this PR is already big.