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 examples for pip and poetry #358

Merged
merged 19 commits into from
Jan 27, 2025

Conversation

tyler-yankee
Copy link
Collaborator

@tyler-yankee tyler-yankee commented Jan 17, 2025

This addresses #266 by adding two new examples for usage of Drake's Python API:

  • installing Drake by creating a virtual environment and pip install drake
  • installing Drake by creating a poetry project and poetry add drake

Both examples use the Python version of the particle demo found in other examples, and both have CI through GitHub Actions only. The poetry example's GHA script has a separate setup script for Mac because the installation of pipx, which is used to install poetry, is a bit different than on Ubuntu.

This change also implements the logic addressed in #359 in the new install_prereqs scripts.


This change is Reviewable

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


a discussion (no related file):
Okay, I've finished testing the CI (and squashed all my commits from debug). +@BetsyMcPhail for feature review, please?

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


a discussion (no related file):
I followed the logic of drake_cmake_installed when making the install_prereqs scripts, which pulls Drake to /opt/drake/...

I suppose something like drake_cmake_external or drake_bazel_external, which pulls Drake to a local drake-master, would also be fine? What would be preferred here? Ultimately it just needs Drake's install_prereqs script with some differentiating logic depending on Ubuntu vs. Mac.

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


a discussion (no related file):

Previously, tyler-yankee wrote…

I followed the logic of drake_cmake_installed when making the install_prereqs scripts, which pulls Drake to /opt/drake/...

I suppose something like drake_cmake_external or drake_bazel_external, which pulls Drake to a local drake-master, would also be fine? What would be preferred here? Ultimately it just needs Drake's install_prereqs script with some differentiating logic depending on Ubuntu vs. Mac.

This has been updated to NOT download Drake locally, but instead install the necessary system packages via a new custom script. Only required on Ubuntu, though the script for the poetry example needs to install pipx and then install poetry using pipx on both Mac and Linux.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 26 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @tyler-yankee)


.github/workflows/pip.yml line 53 at r3 (raw file):

        env:
          PYTHON_VERSION: '3.10'
        shell: bash

Nit no new line at the end of the file


drake_pip/README.md line 20 at r3 (raw file):

Note: Depending on the system and which version of Python is installed,

Nit remove trailing spaces


drake_pip/README.md line 38 at r3 (raw file):

That's all that is needed to use Drake from Python. See Installation via Pip for more information on installation. For more information on what's available for Drake in Python, see Using Drake from Python and the Python API pydrake.

Nit lines < 80 characters is preferred


private/test/file_sync_test.py line 25 at r3 (raw file):

    ),
    (
        "drake_bazel_download/.github/ubuntu_setup",

The drake_pip and drake_poetry ubuntu_setup scripts can be added here.


private/test/file_sync_test.py line 111 at r3 (raw file):

    )
    for path in [
        "particle.py",

These two files are identical to particle.py and particle_test.py in drake_cmake_installed and drake_cmake_installed_apt. This script should check that all four copies of each stay in sync.


drake_poetry/setup/install_prereqs line 46 at r3 (raw file):


    linux*)
    # Ubuntu-specific installations

The Linux install steps are identical to those in the pip example. Can they be factored out so that they can be added to the file_sync test?


drake_poetry/setup/install_prereqs line 63 at r3 (raw file):

EOF
      )
    elif [[ "$(lsb_release -sc)" == 'noble' ]]; then

Currently, d-e-e is tested on Jammy only. Untested Noble code is likely to become out-of-date. I would consider removing this section and put it back in if/when Noble support is added for this example.

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


.github/workflows/pip.yml line 53 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Nit no new line at the end of the file

Fixed.


drake_pip/README.md line 20 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Nit remove trailing spaces

Fixed.


drake_pip/README.md line 38 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Nit lines < 80 characters is preferred

Forgot to change this today, fixed.


drake_poetry/setup/install_prereqs line 46 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

The Linux install steps are identical to those in the pip example. Can they be factored out so that they can be added to the file_sync test?

Actually, this is my error. In the pip example we have to install python3.10-venv while in the poetry example we have to install pipx. Somehow CI wasn't yelling at me for not installing the former, but it was yesterday....regardless, I'll update these scripts to install those packages, and shouldn't need to factor out because they will be different?


drake_poetry/setup/install_prereqs line 63 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Currently, d-e-e is tested on Jammy only. Untested Noble code is likely to become out-of-date. I would consider removing this section and put it back in if/when Noble support is added for this example.

Understood; changed to only check that we're on Jammy, consistent with the other examples' install_prereqs scripts.


private/test/file_sync_test.py line 25 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

The drake_pip and drake_poetry ubuntu_setup scripts can be added here.

Added.


private/test/file_sync_test.py line 111 at r3 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

These two files are identical to particle.py and particle_test.py in drake_cmake_installed and drake_cmake_installed_apt. This script should check that all four copies of each stay in sync.

Refactored so that the Python files are synced for all four, while the C++ files are synced for only the CMake examples.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 11 files at r4, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @tyler-yankee)


drake_pip/README.md line 55 at r4 (raw file):

cd src
python3 particle_test.py

Nit Missing newline here as well


drake_poetry/setup/install_prereqs line 55 at r4 (raw file):

    fi

    ${maybe_sudo} apt-get update

This is repeated from a few lines above and can be removed.


drake_pip/setup/install_prereqs line 48 at r4 (raw file):

fi

${maybe_sudo} apt-get update

This is repeated from a few lines above and can be removed.

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 unresolved discussions, LGTM missing from assignee betsymcphail, platform LGTM missing (waiting on @BetsyMcPhail)


drake_pip/README.md line 55 at r4 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Nit Missing newline here as well

Fixed.


drake_pip/setup/install_prereqs line 48 at r4 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

This is repeated from a few lines above and can be removed.

Removed.


drake_poetry/setup/install_prereqs line 55 at r4 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

This is repeated from a few lines above and can be removed.

Removed.

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee betsymcphail, platform LGTM missing

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 unresolved discussion, platform LGTM from [betsymcphail]

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

@jwnimmer-tri would you like to review this?

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)

@jwnimmer-tri jwnimmer-tri self-assigned this Jan 21, 2025
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Here's some comments, mainly on the pip one.

Reviewed 13 of 26 files at r1, 8 of 11 files at r4, 2 of 4 files at r5, all commit messages.
Reviewable status: 19 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_pip/README.md line 24 at r5 (raw file):
The idea behind this paragraph is okay, but the specific example is misleading:

For example, call python3.10 on Ubuntu 22.04.

On Ubuntu, python3 is exactly the right command to use -- no minor version suffix. Only on platforms where python3 is broken / unwanted should the user type in the minor version number.

Also, don't link to from_source -- those are the instructions for building Drake from source. The instructions are https://drake.mit.edu/pip.html are what we should be linking to here (if anything).


a discussion (no related file):
Remove the CPPLINT and clang-format files from both new projects. C++ config files are irrelevant for Python projects.


README.md line 35 at r5 (raw file):

Note, the GitHub Actions jobs only build and test `drake_bazel_download`,
`drake_cmake_installed`, `drake_cmake_installed_apt`, `drake_pip`, and
`drake_poetry`, since these are the exemplary cases for lightweight, 

nit Trailing whitespace a end of line.


drake_pip/.github/workflows/ci.yml line 63 at r5 (raw file):

        run: .github/ci_build_test
        env:
          PYTHON_VERSION: '3.10'

BTW Probably this could/should just be '3' instead of '3.10'? We just want to use the default.


drake_pip/.github/ci_build_test line 7 at r5 (raw file):


"python$PYTHON_VERSION" --version
"python$PYTHON_VERSION" -m venv env 

nit Trailing whitespace at end of line.


drake_pip/.github/ci_build_test line 14 at r5 (raw file):

python3 -c 'import pydrake.all; print(pydrake.__file__)'

python3 src/particle_test.py

nit The README says to cd src. If we're going to copy-paste the README text into here, we should keep it exactly the same.


drake_pip/.github/ci_build_test line 17 at r5 (raw file):


deactivate
rm -rf env src/__pycache__

nit Missing newline at end of file.


drake_pip/README.md line 8 at r5 (raw file):

## Instructions

First, install the required Ubuntu packages (this step is not needed on Mac):

Surely something is needed on macOS? They at least need to install a Python interpreter, since Drake is not compatible with Apple Python...

I suppose we might not want to script that installation for them, but at least we should mention it?


drake_pip/README.md line 32 at r5 (raw file):

pip install drake

I imagined that this project would use a requirements.txt file and install using that file (even though it will only have 1 line + comments in it). I believe that's how people are familiar with running their pip-based Python projects?

Also I would have expected that install_prereqs would have automatically created the venv and env/bin/pip3 installed the requirements into it? That would move runnable code out of the README and into scripts, where CI can properly test it. Also, it's less code for the user to type in and make mistakes. All they need to do is install_prereqs and then activate.


drake_pip/README.md line 38 at r5 (raw file):

python3 -c 'import pydrake.all; print(pydrake.file)'

BTW Is this part of the README really adding anything? We might as well just have them run the particle test right away (since it covers a superset of what the import test would do)?


drake_pip/README.md line 50 at r5 (raw file):

## Examples

To run the particle example tests in this directory, navigate to `src` and call the test file to execute the unit tests:

nit Line wrap to <= 80 characters.


drake_pip/setup/install_prereqs line 3 at r5 (raw file):

#!/bin/bash

# Copyright (c) 2020, Massachusetts Institute of Technology.

We are trying to git rid of this old license (BSD-3-Clause).

Please re-implement this file from scratch (without reference to the BSD-3-Clause file you copied it from, so that we don't need to preserve the old file's license), so this file can have the "SPDX ... MIT-0" comment atop it, instead.


drake_pip/setup/install_prereqs line 43 at r5 (raw file):

${maybe_sudo} apt-get install --no-install-recommends lsb-release

if [[ "$(lsb_release -sc)" != 'jammy' ]]; then

We don't want this to die here. Unlike all of our other examples, the pip (and poetry) examples should work on a lot of different OS's.

Obviously this script currently only works on some kind of Debian or Ubuntu, not e.g. RedHat, but we don't need to check for that. The documentation can caution users if we need to.


drake_pip/setup/install_prereqs line 49 at r5 (raw file):


${maybe_sudo} apt-get install --no-install-recommends $(cat <<EOF
  python3-all-dev

nit The dev package seems like a lot more than we need? I would think we only want python3 by itself?


drake_pip/setup/install_prereqs line 54 at r5 (raw file):

  libsm6
  libglib2.0-0
  python3.10-venv

nit Spell this as python3-venv to match the python3(-dev) default version above. We are happy to use whatever version is Ubuntu's default.

Also keep the list alpha-sorted for clarity.


drake_pip/LICENSE line 1 at r5 (raw file):

Copyright (c) 2017-2023 by the drake-external-examples developers.

nit This needs to be either 2017-2025 or just 2025, but can't end at 2023 since the code new is being written today.

It's fine to change all of the other LICENSE files in the repo to be 2017-2025 at the same time.


drake_poetry/LICENSE line 1 at r5 (raw file):

Copyright (c) 2017-2023 by the drake-external-examples developers.

nit Ditto 2017-2025.


drake_poetry/README.md line 3 at r5 (raw file):

# Python Project with Drake Installed from Poetry

This installs Drake using [`poetry`](https://python-poetry.org/), the Python package manager.

nit Line wrap to <= 80 characters throughout this file.


drake_poetry/README.md line 34 at r5 (raw file):

The pyproject.toml file that's created contains the project and dependency information.

This README comes off as a bit weird to me. It feels more like a one-time intro / tutorial document, instead of a project that's already alive.

I imagined we would check in the pyproject.toml file so that everything is ready to go. We can still have comments saying how to recreate/update it, but we want the example projects to be like something a user would actually live with on an ongoing basis. It should be ready to fork and be immediately off to the races. Only if the user wants to change something should it require any extra work.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 19 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_pip/setup/install_prereqs line 3 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We are trying to git rid of this old license (BSD-3-Clause).

Please re-implement this file from scratch (without reference to the BSD-3-Clause file you copied it from, so that we don't need to preserve the old file's license), so this file can have the "SPDX ... MIT-0" comment atop it, instead.

Actually nevermind, it's simpler than that. See #361.

You don't need to start over, you can just replace this comment with the SPDX one-liner comment.

@tyler-yankee tyler-yankee force-pushed the pypi-examples branch 3 times, most recently from 731e79d to ab4f1b5 Compare January 22, 2025 19:58
@tyler-yankee
Copy link
Collaborator Author

drake_poetry/README.md line 34 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This README comes off as a bit weird to me. It feels more like a one-time intro / tutorial document, instead of a project that's already alive.

I imagined we would check in the pyproject.toml file so that everything is ready to go. We can still have comments saying how to recreate/update it, but we want the example projects to be like something a user would actually live with on an ongoing basis. It should be ready to fork and be immediately off to the races. Only if the user wants to change something should it require any extra work.

Okay, yeah perhaps I didn't understand the original intention/philosophy behind the example project. I just refactored it where the pyproject.toml and poetry.lock files are checked into the repo and the README is modified to contain a description of the script and simple instructions to install Drake from it.

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @BetsyMcPhail and @jwnimmer-tri)


drake_pip/README.md line 53 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

read below for a quick overview of the steps.

I don't think we should copy-paste the bash script into the README, that's too brittle. If users want to see what it's doing, they can read (or we can point them to) the script contents. It would be okay to give a summary of the ideas of the script here if we like, but pasting the code goes too far.

Understood. Removed this overview section and moved the relevant links to the top.


drake_pip/requirements.txt line 3 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Missing newline at end of file.

Fixed.


drake_pip/setup/install_prereqs line 13 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The lsb-release is vestigial now.

Removed.


drake_pip/setup/install_prereqs line 15 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit To improve clarity for DEE maintainers, we should have a comment here that cites https://github.com/RobotLocomotion/drake/blob/master/tools/wheel/content/INSTALLATION as the authority the required libraries.

(BTW the libegl1 is still required here, even though not listed in that file, until the next drake stable release mid-February.)

Added.


drake_pip/setup/install_prereqs line 25 at r7 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If the user runs setup/install_prereqs twice in a row, does it succeed? That's a common use case (to refresh any missing dependencies as the list of packages / requirements evolves over time).

Works on my machine.

The only caveat would be that the venv would be created a second time, so if the user added any other packages manually and didn't include them in requirements.txt, then they would be lost. I'd say that's on them for not including in requirements.txt and rogue-calling the script, but what are your thoughts?

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

The pip example LGTM. Just a few notes on the poetry now that I've time to read it ...

Reviewed 11 of 12 files at r8, 4 of 4 files at r9, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_pip/setup/install_prereqs line 25 at r7 (raw file):

I'd say that's on them for not including in requirements.txt and rogue-calling the script, but what are your thoughts?

Agreed. The complexity of dealing with that situation would be a net-negative in what's supposed to be a simple example. We can leave this as-is.


drake_poetry/setup/install_prereqs line 20 at r9 (raw file):

    # Ubuntu-specific installations
    ${maybe_sudo} apt-get update
    ${maybe_sudo} apt-get install --no-install-recommends lsb-release

nit Ditto on not using lsb-release and removing the jammy-check, below.


drake_poetry/setup/install_prereqs line 27 at r9 (raw file):

    fi

    ${maybe_sudo} apt-get install --no-install-recommends $(cat <<EOF

nit Ditto on xref to the INSTALLATION file.


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):


# Install poetry
pipx install poetry

Where does this install poetry into? It looks like probably "globally" i.e. $HOME.

That breaks one of the invariants of our examples -- that any changes to the computer must be isolated to either:

  • happen via the host's package manager (brew or apt), or
  • happen exclusively inside the DEE source tree.

It's important that nothing we do here would "pollute" other projects the user has in their homedir, so we can't put things into $HOME directly.


drake_poetry/.github/workflows/ci.yml line 58 at r9 (raw file):

      # ubuntu_setup and ci_build_test must occur in one step
      # because when poetry is installed, the update to PATH
      # does not persist between steps on GHA.

This is not a good outcome. The "run build and test" should able to happen separately from any setup activities, otherwise when the user reboots and tries again tomorrow, their code won't work anymore. It should not be incumbent on them to run install_prereqs every time they log in to their machine, or open a new terminal, or etc.

Code quote:

      # ubuntu_setup and ci_build_test must occur in one step
      # because when poetry is installed, the update to PATH
      # does not persist between steps on GHA.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Where does this install poetry into? It looks like probably "globally" i.e. $HOME.

That breaks one of the invariants of our examples -- that any changes to the computer must be isolated to either:

  • happen via the host's package manager (brew or apt), or
  • happen exclusively inside the DEE source tree.

It's important that nothing we do here would "pollute" other projects the user has in their homedir, so we can't put things into $HOME directly.

Ah, I see now that https://python-poetry.org/docs/#installing-with-pipx says that pipx does use a venv? So that's okay.

But maybe it still barfs in our homedir's dotfiles, thus the need for sourcing $HOME/.profile, below?

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, I see now that https://python-poetry.org/docs/#installing-with-pipx says that pipx does use a venv? So that's okay.

But maybe it still barfs in our homedir's dotfiles, thus the need for sourcing $HOME/.profile, below?

Yeah, it uses a venv but aliases poetry to $HOME/.local/bin/poetry.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, tyler-yankee wrote…

Yeah, it uses a venv but aliases poetry to $HOME/.local/bin/poetry.

Hopefully there's a flag to opt-out of that?

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/.github/workflows/ci.yml line 58 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is not a good outcome. The "run build and test" should able to happen separately from any setup activities, otherwise when the user reboots and tries again tomorrow, their code won't work anymore. It should not be incumbent on them to run install_prereqs every time they log in to their machine, or open a new terminal, or etc.

I believe this is unique to GHA, i.e., it does not maintain a profile between steps so PATH updates don't persist. Any actual user would only need to run install_prereqs to install poetry and the packages, and then once it's on PATH they're good just as any other executable would be.

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Hopefully there's a flag to opt-out of that?

From the pipx docs: "The default binary location for pipx-installed apps is ~/.local/bin. This can be overridden with the environment variable PIPX_BIN_DIR." "pipx's default virtual environment location is typically ~/.local/share/pipx on Linux/Unix, ~/.local/pipx on MacOS and ~\pipx on Windows. This can be overridden with the PIPX_HOME environment variable."

So it looks like we may have options. There is also the option to use the official installer instead of pipx. This one installs to the default python3 bin. I chose pipx initially because it was the first option on the website and I wanted to stick to their preferred installation methods as much as possible.

Copy link
Collaborator Author

@tyler-yankee tyler-yankee left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @jwnimmer-tri)


drake_poetry/setup/install_prereqs line 20 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Ditto on not using lsb-release and removing the jammy-check, below.

Removed.


drake_poetry/setup/install_prereqs line 27 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Ditto on xref to the INSTALLATION file.

Added.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r10, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/.github/workflows/ci.yml line 58 at r9 (raw file):

Previously, tyler-yankee wrote…

I believe this is unique to GHA, i.e., it does not maintain a profile between steps so PATH updates don't persist. Any actual user would only need to run install_prereqs to install poetry and the packages, and then once it's on PATH they're good just as any other executable would be.

The problem is that DEE is never allowed to permanently alter their $PATH in the first-place. It probably makes sense to resolve the other thread about how we install poetry before circling back here.


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

I wanted to stick to their preferred installation methods as much as possible.

This is the right principle.

So the question is, what is the best way to install poetry (most traditional, and/or most bulletproof) that doesn't write into $HOME (modulo that stashing files in $HOME/.cache to avoid re-downloading is okay)? While also keeping the example as simple / straightforward as possible

I don't know Poetry so I don't know what to recommend. I'll have to go with your best guess, or we can ask on Drake slack for users' opinions.

If we end up with a slightly-odd install of poetry, we could certainly mention in our README doc that we're being extra careful to not disturb $HOME, but that users are welcome to install it how they prefer. Presumably if they are reading this example, they already know how Poetry works.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I wanted to stick to their preferred installation methods as much as possible.

This is the right principle.

So the question is, what is the best way to install poetry (most traditional, and/or most bulletproof) that doesn't write into $HOME (modulo that stashing files in $HOME/.cache to avoid re-downloading is okay)? While also keeping the example as simple / straightforward as possible

I don't know Poetry so I don't know what to recommend. I'll have to go with your best guess, or we can ask on Drake slack for users' opinions.

If we end up with a slightly-odd install of poetry, we could certainly mention in our README doc that we're being extra careful to not disturb $HOME, but that users are welcome to install it how they prefer. Presumably if they are reading this example, they already know how Poetry works.

Ah. It looks like both Ubuntu and Homebrew already have poetry packages? Maybe we should just install that?

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah. It looks like both Ubuntu and Homebrew already have poetry packages? Maybe we should just install that?

I'm not seeing where Ubuntu has a poetry package, and the Homebrew poetry package is not recommended as you get much less control over the versions of both poetry itself and Python that you can use. I don't think we should install that.

Actually, it looks like pipx has a--global argument to install packages to /usr/local/bin. (It's not supported on Windows, though we're not dealing with Windows at present.) We can either use that, or do a custom install with pipx to something like /opt/poetry. This won't touch $HOME or $PATH, and just means we'll have to specify the location of poetry throughout scripts. We can detail this in the README and even include the code if users wanted to modify their $PATH as a suggestion. Do you have a preference for either option?

(P.S.: The official installer retrieved with curl also has an option to specify a custom directory, but pipx allows you to more readily manage versions or uninstall.)

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, tyler-yankee wrote…

I'm not seeing where Ubuntu has a poetry package, and the Homebrew poetry package is not recommended as you get much less control over the versions of both poetry itself and Python that you can use. I don't think we should install that.

Actually, it looks like pipx has a--global argument to install packages to /usr/local/bin. (It's not supported on Windows, though we're not dealing with Windows at present.) We can either use that, or do a custom install with pipx to something like /opt/poetry. This won't touch $HOME or $PATH, and just means we'll have to specify the location of poetry throughout scripts. We can detail this in the README and even include the code if users wanted to modify their $PATH as a suggestion. Do you have a preference for either option?

(P.S.: The official installer retrieved with curl also has an option to specify a custom directory, but pipx allows you to more readily manage versions or uninstall.)

Ubuntu has https://packages.ubuntu.com/jammy/python3-poetry.

Using global /usr/local is even worse than $HOME, so that's a no-go. We can either use the OS package manager, or we can install exclusively inside the DEE folders.

Given that rule, we aren't going to be able to use one of poetry's "recommended" mechanisms, since it sounds like all of them will necessarily will update $HOME and/or $PATH since they want to make things easy to use. So I think no matter what, this example won't be turnkey -- our scripting will install it one way, but the text of the README will point out the option to install poetry one of the more conventional ways.

Maybe a good solution is to move the poetry install into the .github code exclusively (not setup/install_prereqs) and then in the README just say "install poetry somehow, first". In the .github code we install it however we like, the rule about not touching $HOME is only for the user-facing setup/install_prereqs automation.

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):
I like this idea, though the poetry docs warn the following:

Poetry should always be installed in a dedicated virtual environment to isolate it from the rest of your system. Each of the above described installation methods ensures that. It should in no case be installed in the environment of the project that is to be managed by Poetry. This ensures that Poetry’s own dependencies will not be accidentally upgraded or uninstalled. In addition, the isolated virtual environment in which poetry is installed should not be activated for running poetry commands.

It's unclear to me how we're reconciling the updating of $PATH from the user side vs. the CI side with this approach. If CI is completely divided from what users are doing then it defeats the purpose. But then how is poetry called in setup/install_prereqs?

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM from [betsymcphail] (waiting on @tyler-yankee)


drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

... installed in a dedicated virtual environment to isolate it from the rest of your system ...

This is good advice for everyday human users. In CI, we are effectively already in virtual environment of a sort, since CI is running isolated from anything else, with only our software installed.

In any case, CI-installing with pipx is still fine, if that seems best. Once the CI scripts are doing it, we can do it any way we want.

If CI is completely divided from what users are doing then it defeats the purpose.

This is an overstatement. Out goal is to give the best example we can. Given the requirements, telling the user install poetry instead of automating it seems like a reasonable compromise. The example will still show what the toml file looks like, how to run programs, etc.

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

... installed in a dedicated virtual environment to isolate it from the rest of your system ...

This is good advice for everyday human users. In CI, we are effectively already in virtual environment of a sort, since CI is running isolated from anything else, with only our software installed.

In any case, CI-installing with pipx is still fine, if that seems best. Once the CI scripts are doing it, we can do it any way we want.

If CI is completely divided from what users are doing then it defeats the purpose.

This is an overstatement. Out goal is to give the best example we can. Given the requirements, telling the user install poetry instead of automating it seems like a reasonable compromise. The example will still show what the toml file looks like, how to run programs, etc.

Fair enough. How does the following sound, then:

  • moving the poetry and pipx installation out of setup/install_prereqs into .github scripts
  • all that's left in install_prereqs is poetry install (to install Drake), so we could delete that script and just tell users to run that command in the README
  • for CI, keeping the existing pipx install but to some /opt/poetry so that the code is cleaner without having to source ~/.profile all the time

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, tyler-yankee wrote…

Fair enough. How does the following sound, then:

  • moving the poetry and pipx installation out of setup/install_prereqs into .github scripts
  • all that's left in install_prereqs is poetry install (to install Drake), so we could delete that script and just tell users to run that command in the README
  • for CI, keeping the existing pipx install but to some /opt/poetry so that the code is cleaner without having to source ~/.profile all the time

Well, we still need install_prereqs for the system packages on Ubuntu. I'll have a refactor with the rest of these changes coming on Monday.

@tyler-yankee
Copy link
Collaborator Author

drake_poetry/setup/install_prereqs line 40 at r9 (raw file):

Previously, tyler-yankee wrote…

Well, we still need install_prereqs for the system packages on Ubuntu. I'll have a refactor with the rest of these changes coming on Monday.

Updated the user instructions, user script (install_prereqs), and CI scripts (setup) to reflect the above discussion.

Copy link
Contributor

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r11, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [betsymcphail, jwnimmer-tri] (waiting on @tyler-yankee)

@jwnimmer-tri jwnimmer-tri merged commit 3ea516d into RobotLocomotion:main Jan 27, 2025
11 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