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.

Sign up for GitHub

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

Jump to bottom

Migrate CI dependency installation from pip to uv #3675

Merged
merged 5 commits into from
Mar 7, 2024
Merged

Migrate CI dependency installation from pip to uv #3675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db29ef6
switch test.yml GHA workflow dependency installation from pip to uv
janosh Mar 7, 2024
ccb7254
remove unnecessary editable install from source build in release work…
janosh Mar 7, 2024
1850b86
fix typo
janosh Mar 7, 2024
6a9d3c5
remove env vars MPLBACKEND, PMG_TEST_FILES_DIR to check if still nece…
janosh Mar 7, 2024
4fcbc58
skip failing m3gnet tests with TODO and link to matgl issue
janosh Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@ jobs:
with:
python-version: "3.11"

- run: |
python -m pip install build
pip install -e .

- name: Build sdist
run: python -m build --sdist
run: |
pip install build
python -m build --sdist

- uses: actions/upload-artifact@v3
with:
Expand Down
25 changes: 16 additions & 9 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ on:
branches: [master]
paths-ignore: ["**/*.md", docs/**]
workflow_dispatch:
# make this workflow reusable by release.yml
workflow_call:
workflow_call: # make this workflow reusable by release.yml

permissions:
contents: read
Expand Down Expand Up @@ -42,30 +41,36 @@ jobs:

env:
PMG_MAPI_KEY: ${{ secrets.PMG_MAPI_KEY }}
MPLBACKEND: Agg # https://github.com/orgs/community/discussions/26434
PMG_TEST_FILES_DIR: ${{ github.workspace }}/tests/files
GULP_LIB: ${{ github.workspace }}/cmd_line/gulp/Libraries
PMG_VASP_PSP_DIR: ${{ github.workspace }}/tests/files

steps:
- uses: actions/checkout@v4
- name: Check out repo
uses: actions/checkout@v4

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: setup.py

- name: Install uv
run: pip install uv

- name: Copy GULP to bin
if: matrix.os == 'ubuntu-latest'
run: |
sudo cp cmd_line/gulp/Linux_64bit/* /usr/local/bin/

- name: Install Bader
if: matrix.os == 'ubuntu-latest'
run: |
wget https://theory.cm.utexas.edu/henkelman/code/bader/download/bader_lnx_64.tar.gz
tar xvzf bader_lnx_64.tar.gz
sudo mv bader /usr/local/bin/
continue-on-error: true # This is not critical to succeed.

- name: Install Enumlib
if: matrix.os == 'ubuntu-latest'
run: |
Expand All @@ -79,6 +84,7 @@ jobs:
cd ..
sudo cp aux_src/makeStr.py /usr/local/bin/
continue-on-error: true # This is not critical to succeed.

- name: Install Packmol
if: matrix.os == 'ubuntu-latest'
run: |
Expand All @@ -91,14 +97,15 @@ jobs:
sudo mv packmol /usr/local/bin/
cd ..
continue-on-error: true # This is not critical to succeed.

- name: Install dependencies
run: |
python -m pip install numpy cython
uv pip install numpy cython --system

# TODO remove next line installing ase from main branch until FrechetCellFilter is released
pip install git+https://gitlab.com/ase/ase
uv pip install -e '.[dev,optional]' --system
Copy link
Contributor

Choose a reason for hiding this comment

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

@janosh, sorry for digging out this. But why do we need to install "editable" for CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't remember exactly. i'm fairly sure i tried non-editable first and it failed for some reason. but that was with an earlier uv and might have changed

Copy link
Contributor

@DanielYang59 DanielYang59 Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I could confirm install "non-editable" indeed breaks the tests, and install editable works fine. The pip install process seems to run without any issue, but pytest seems unable to access test files. I don't quite understand this behavior though (thought the only difference being able to change source code).

An off-topic question: why do we need:

pymatgen/.github/workflows/test.yml

Lines 18 to 21 in 7064c43

jobs:
test:
# prevent this action from running on forks
if: github.repository == 'materialsproject/pymatgen'

By default workflows would not run on forks until enabled by fork owner. Having this would prevent fork from running workflows, which seems unnecessary (some people might prefer to test on their own fork before pushing to upstream)? Or in my case I have to remove this line to run tests on my own fork.

Screenshot 2024-04-10 at 16 57 58

Copy link
Member Author

Choose a reason for hiding this comment

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

editable install basically just symlinks the repo into site-packages, meaning the whole repo contents including test files are symlinked. i think with non-editable install TEST_FILES_DIR points at a non-existent directory.

why do we need:

     if: github.repository == 'materialsproject/pymatgen'

not sure if this changed but people used to get every CI failure email twice from GitHub, once on their fork and once on open PRs which was very annoying

Copy link
Contributor

@DanielYang59 DanielYang59 Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

editable install basically just symlinks the repo into site-packages, meaning the whole repo contents including test files are symlinked. i think with non-editable install TEST_FILES_DIR points at a non-existent directory.

That's interesting. I might experiment with this later. Because I have a feeling that we don't need install editable when we don't actually edit the source code. And I'm not sure if install editable would have any performance penalty. It's just guessing now but I would test later and let you know :)

not sure if this changed but people used to get every CI failure email twice from GitHub, once on their fork and once on open PRs which was very annoying

I think this is intended (one failure from their own fork, and one from upstream). They should disable workflow on their own fork if they don't want workflow to run on their fork, instead of we disable altogether for them? Otherwise people who want to run workflow on their fork for any reason would need to remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm guessing if anything, --editable offers a performance gain given how fast it is to symlink

Otherwise people who want to run workflow on their fork for any reason would need to remove this line?

not sure that's an issue? people usually just fork to make PRs in which CI runs on this repo. if they want to maintain a fork long-term, removing this line is not much work?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm guessing if anything, --editable offers a performance gain given how fast it is to symlink

Not sure, but I would test later and let you know.

people usually just fork to make PRs in which CI runs on this repo.

Yes. But my point is... By default workflow would NOT run on forks (I assume people would enable workflow on their fork only when they have a good reason to), such that people could easily toggle workflow on and off easily on their fork (and have more control over their fork).

But if we disable workflow for them, they can not easily enable it. (I guess click a button would be easier than changing a line and make a commit? Also if they want to to push their change to upstream, they would have to revert it.)


python -m pip install -e '.[dev,optional]'
# TODO remove next line installing ase from main branch when FrechetCellFilter is released
uv pip install --upgrade 'ase@git+https://gitlab.com/ase/ase' --system

- name: pytest split ${{ matrix.split }}
run: |
Expand Down
20 changes: 10 additions & 10 deletions pymatgen/transformations/advanced_transformations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,9 @@ def __init__(
if max_cell_size and max_disordered_sites:
raise ValueError("Cannot set both max_cell_size and max_disordered_sites!")

def apply_transformation(self, structure: Structure, return_ranked_list: bool | int = False):
def apply_transformation(
self, structure: Structure, return_ranked_list: bool | int = False
) -> Structure | list[dict]:
"""Returns either a single ordered structure or a sequence of all ordered
structures.

Expand Down Expand Up @@ -879,7 +881,7 @@ def apply_transformation(
# remove dummy species and replace Spin.up or Spin.down
# with spin magnitudes given in mag_species_spin arg
alls = self._remove_dummy_species(alls)
alls = self._add_spin_magnitudes(alls)
alls = self._add_spin_magnitudes(alls) # type: ignore[arg-type]
else:
for idx in range(len(alls)):
alls[idx]["structure"] = self._remove_dummy_species(alls[idx]["structure"])
Expand All @@ -891,7 +893,7 @@ def apply_transformation(
num_to_return = 1

if num_to_return == 1 or not return_ranked_list:
return alls[0]["structure"] if num_to_return else alls
return alls[0]["structure"] if num_to_return else alls # type: ignore[return-value]

# remove duplicate structures and group according to energy model
matcher = StructureMatcher(comparator=SpinComparator())
Expand Down Expand Up @@ -1010,11 +1012,10 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool |
Args:
structure (Structure): Input structure to dope
return_ranked_list (bool | int, optional): If return_ranked_list is int, that number of structures.

is returned. If False, only the single lowest energy structure is returned. Defaults to False.

Returns:
[{"structure": Structure, "energy": float}]
list[dict] | Structure: each dict has shape {"structure": Structure, "energy": float}.
"""
comp = structure.composition
logger.info(f"Composition: {comp}")
Expand Down Expand Up @@ -1059,7 +1060,7 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool |
logger.info(f"{lengths=}")
logger.info(f"{scaling=}")

all_structures = []
all_structures: list[dict] = []
trafo = EnumerateStructureTransformation(**self.kwargs)

for sp in compatible_species:
Expand Down Expand Up @@ -1131,10 +1132,9 @@ def apply_transformation(self, structure: Structure, return_ranked_list: bool |
}
)

ss = trafo.apply_transformation(supercell, return_ranked_list=self.max_structures_per_enum)
logger.info(f"{len(ss)} distinct structures")
all_structures.extend(ss)

structs = trafo.apply_transformation(supercell, return_ranked_list=self.max_structures_per_enum)
logger.info(f"{len(structs)} distinct structures")
all_structures.extend(structs)
logger.info(f"Total {len(all_structures)} doped structures")
if return_ranked_list:
return all_structures[:return_ranked_list]
Expand Down
4 changes: 4 additions & 0 deletions tests/core/test_structure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,6 +1705,7 @@ def test_relax_ase_opt_kwargs(self):
assert traj[0] != traj[-1]
assert os.path.isfile(traj_file)

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_calculate_m3gnet(self):
pytest.importorskip("matgl")
calculator = self.get_structure("Si").calculate()
Expand All @@ -1716,6 +1717,7 @@ def test_calculate_m3gnet(self):
assert np.linalg.norm(calculator.results["forces"]) == approx(7.8123485e-06, abs=0.2)
assert np.linalg.norm(calculator.results["stress"]) == approx(1.7861567, abs=2)

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_relax_m3gnet(self):
pytest.importorskip("matgl")
struct = self.get_structure("Si")
Expand All @@ -1726,6 +1728,7 @@ def test_relax_m3gnet(self):
actual = relaxed.dynamics[key]
assert actual == val, f"expected {key} to be {val}, {actual=}"

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_relax_m3gnet_fixed_lattice(self):
pytest.importorskip("matgl")
struct = self.get_structure("Si")
Expand All @@ -1734,6 +1737,7 @@ def test_relax_m3gnet_fixed_lattice(self):
assert hasattr(relaxed, "calc")
assert relaxed.dynamics["optimizer"] == "BFGS"

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_relax_m3gnet_with_traj(self):
pytest.importorskip("matgl")
struct = self.get_structure("Si")
Expand Down
6 changes: 4 additions & 2 deletions tests/transformations/test_advanced_transformations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ def test_apply_transformation(self):
for s in alls:
assert "energy" not in s

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_m3gnet(self):
pytest.importorskip("matgl")
enum_trans = EnumerateStructureTransformation(refine_structure=True, sort_criteria="m3gnet_relax")
Expand All @@ -204,6 +205,7 @@ def test_m3gnet(self):
# Check ordering of energy/atom
assert alls[0]["energy"] / alls[0]["num_sites"] <= alls[-1]["energy"] / alls[-1]["num_sites"]

@pytest.mark.skip("TODO remove skip once https://github.com/materialsvirtuallab/matgl/issues/238 is resolved")
def test_callable_sort_criteria(self):
matgl = pytest.importorskip("matgl")
from matgl.ext.ase import Relaxer
Expand All @@ -212,8 +214,8 @@ def test_callable_sort_criteria(self):

m3gnet_model = Relaxer(potential=pot)

def sort_criteria(s):
relax_results = m3gnet_model.relax(s)
def sort_criteria(struct: Structure) -> tuple[Structure, float]:
relax_results = m3gnet_model.relax(struct)
energy = float(relax_results["trajectory"].energies[-1])
return relax_results["final_structure"], energy

Expand Down