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

ci update #1305

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

ci update #1305

wants to merge 12 commits into from

Conversation

sjsrey
Copy link
Member

@sjsrey sjsrey commented May 12, 2024

Addressing #1299

@sjsrey sjsrey requested a review from jGaboardi May 12, 2024 15:33
@sjsrey sjsrey added WIP Work in progress, do not merge. Discussion only. CI labels May 12, 2024
@sjsrey sjsrey requested a review from knaaptime May 12, 2024 15:40
@sjsrey
Copy link
Member Author

sjsrey commented May 12, 2024

This was modeled after spopt ci, for reference.

.github/workflows/testing.yml Show resolved Hide resolved
- inequality
- access
- splot
- pip:
Copy link
Member

Choose a reason for hiding this comment

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

Shall we move spopt into the dependencies section (in all environments)?

Copy link
Member

Choose a reason for hiding this comment

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

Gentle ping to remind to remove the pip: spopt and move spopt directly up beneath splot in an CI environments for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

For the -oldest tag we should ideally have minimum pins. We could probably simply follow SPEC000 for this.

Copy link
Member

Choose a reason for hiding this comment

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

That is a bit tricky to do in here as you would need to pin 2nd order dependencies, given the pysal depends only on subpackages, where we always want to pull the latest, never the oldest. I would probably leave this env out.

Copy link
Member

Choose a reason for hiding this comment

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

@sjsrey If we follow @martinfleis's idea here, we can simply rename 310-oldest.yaml --> 310-latest.yaml (and in the workflow file) and have done with it.

.github/workflows/testing.yml Outdated Show resolved Hide resolved
ci/312-dev.yaml Outdated Show resolved Hide resolved
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
sjsrey and others added 2 commits May 12, 2024 14:07
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>
@jGaboardi
Copy link
Member

Looks like pysal isn't actually being installed here?

_________________ ERROR collecting pysal/tests/test_imports.py _________________
ImportError while importing test module '/home/runner/work/pysal/pysal/pysal/tests/test_imports.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../micromamba/envs/test/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
pysal/tests/test_imports.py:2: in <module>
    from pysal import federation_hierarchy
E   ModuleNotFoundError: No module named 'pysal'

@jGaboardi
Copy link
Member

re: pacakage install

For some packages (e.g. spopt and spaghetti), merely checking out the repo is enough. However, this is not the case most of time (e.g. tobler and momepy). I have never been able to figure the difference and why it is not needed sometimes. Seems like it is needed here.

Copy link
Member

Choose a reason for hiding this comment

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

Is this file still even needed? I opened #1306 because I thought it could be pruned entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI WIP Work in progress, do not merge. Discussion only.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants