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

Migrate from poetry to uv #454

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

Migrate from poetry to uv #454

wants to merge 4 commits into from

Conversation

icui
Copy link
Collaborator

@icui icui commented Feb 5, 2025

Closes #161

@icui icui requested review from lsawade and Rohit-Kakodkar and removed request for Rohit-Kakodkar February 5, 2025 23:32
@lsawade
Copy link
Collaborator

lsawade commented Feb 6, 2025

gotta

git checkout devel && git pull origin devel && git checkout issue-161 && git merge devel

Copy link
Collaborator

@lsawade lsawade left a comment

Choose a reason for hiding this comment

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

Why is the new clang-format not introducing the slew of formatting changes?


Install SPECFEM++ development environment
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: bash

poetry install
uv sync --extra dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you not need the extra line for the code-block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't run clang-format for all files yet. Maybe do it after we merge devel to main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you run make html in docs, there are a lot of errors but most errors are related to formatting and tell you where the underlines are too short/long

Copy link
Collaborator

@lsawade lsawade left a comment

Choose a reason for hiding this comment

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

Title underlines, this is why rst is kinda annoying sometimes


Install poetry
Install uv
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the underline for headers in RST files require the exact length of the title string

~~~~~~~~~~~~~~~

Download and install poetry using the `official instructions <https://python-poetry.org/docs/#installation>`_.
Download and install uv using the `official instructions <https://docs.astral.sh/uv/getting-started/installation>`_.

Install SPECFEM++ development environment
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too, but that wasn't you. A lot of the docs are probs written by copilot, and copilot is not doing it right.


Using your python/poetry environment
Using your python/uv environment
Copy link
Collaborator

Choose a reason for hiding this comment

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

here


.. code-block:: bash

poetry run python <script_name>.py
source .venv/bin/activate

Pre-commit hooks
~~~~~~~~~~~~~~~~~
Copy link
Collaborator

Choose a reason for hiding this comment

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

here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@icui icui requested a review from lsawade February 7, 2025 20:47
@@ -15,11 +15,11 @@ To make a change to SPECFEM++ (This development workflow is similar to the one s

.. note::

It is also recommended that you run :code:`poetry install` every time you pull the develop branch. Please check :ref:`style section<style>` for more information on poetry.
It is also recommended that you run :code:`uv sync --extra dev` every time you pull the develop branch. Please check :ref:`style section<style>` for more information on uv.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The underline in this file's header is too long

@@ -4,69 +4,59 @@ Style
Pre-commit
----------

SPECFEM++ uses pre-commit to check style. Pre-commit can be installed inside python virtual environments. There are several methods for creating virtual environments, but I've found poetry tool is great at managing python environments. Especailly in collaborative environments poetry gives easy method for managing consistency of environments between all contributors via :code:`pyproject.toml` and :code:`poetry.lock` files.
SPECFEM++ uses pre-commit to check style. Pre-commit can be installed inside python virtual environments. There are several methods for creating virtual environments, but I've found uv tool is great at managing python environments. Especailly in collaborative environments uv gives easy method for managing consistency of environments between all contributors via :code:`pyproject.toml` and :code:`uv.lock` files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Header here at the very top is too long.


Install SPECFEM++ development environment
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: bash

poetry install
uv sync --extra dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you run make html in docs, there are a lot of errors but most errors are related to formatting and tell you where the underlines are too short/long

@icui icui requested a review from Rohit-Kakodkar February 24, 2025 15:07
Copy link
Collaborator

@Rohit-Kakodkar Rohit-Kakodkar left a comment

Choose a reason for hiding this comment

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

LGTM!

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