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

modernize infrastructure #121

Merged
merged 1 commit into from
Sep 21, 2023
Merged

modernize infrastructure #121

merged 1 commit into from
Sep 21, 2023

Conversation

knaaptime
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #121 (0fe03b2) into main (4ff0b0d) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #121   +/-   ##
=====================================
  Coverage   69.1%   69.1%           
=====================================
  Files         40      40           
  Lines       8723    8723           
  Branches    1195    1253   +58     
=====================================
  Hits        6027    6027           
  Misses      2316    2316           
  Partials     380     380           
Files Changed Coverage Δ
spreg/diagnostics_panel.py 96.0% <100.0%> (ø)

@knaaptime
Copy link
Member Author

@pedrovma this should be ready. Doesnt touch any substantive code, but brings infrastructure in line with the rest of the ecosystem

@pedrovma
Copy link
Member

This is great! Thank you so much.

@pedrovma pedrovma merged commit 7e8c7b0 into pysal:main Sep 21, 2023
9 checks passed
default: test
required: false
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

Do we still have a master here?

.github/workflows/build_docs.yml Show resolved Hide resolved
strategy:
matrix:
os: ['ubuntu-latest']
environment-file: [ci/310.yaml]
environment-file: [.ci/39.yml]
Copy link
Member

Choose a reason for hiding this comment

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

Why downgrade to 39 from 310 here? Probably should actually be 311.

"pytest-mpl",
"pytest-cov",
"watermark",

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

]
requires-python = ">=3.8"
dependencies = [
"scipy>=0.11",
Copy link
Member

Choose a reason for hiding this comment

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

Needs (desires?) indent.

"Intended Audience :: Science/Research",
"Topic :: Scientific/Engineering :: GIS",
]
requires-python = ">=3.8"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
requires-python = ">=3.8"
requires-python = ">=3.9"

.github/workflows/release_and_publish.yml Show resolved Hide resolved
- name: setup micromamba
uses: mamba-org/provision-with-micromamba@main
- uses: actions/checkout@v2
- uses: actions/cache@v2
Copy link
Member

Choose a reason for hiding this comment

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

What's the need for this cache action?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure actually

import sphinx_bootstrap_theme


sys.path.insert(0, os.path.abspath("../"))
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed? It seems to still be needed in spaghetti.

Copy link
Member Author

Choose a reason for hiding this comment

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

im not sure about this. I've had to remove it from tobler and segregation otherwise the docs build will fail

Copy link
Member

Choose a reason for hiding this comment

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

hmmm OK.

- name: commit docs
path: ~/conda_pkgs_dir
key: ${{ matrix.os }}-conda-${{ env.CACHE_NUMBER }}-${{ hashFiles(matrix.environment-file) }}
- uses: conda-incubator/setup-miniconda@v2
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for not using the mamba-org/setup-micromamba@v1 action here?

Copy link
Member Author

Choose a reason for hiding this comment

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

no... which package has the most current recipes?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question... seems to be a hodgepodge

@knaaptime
Copy link
Member Author

thanks for the close review james! All of the recipes are basically just copied over from tobler, so there may be cases like v3 vs v4 that can be directly updated.

@jGaboardi
Copy link
Member

thanks for the close review james! All of the recipes are basically just copied over from tobler, so there may be cases like v3 vs v4 that can be directly updated.

LOL I can read this as "dang it, James".

Those versions are probably not going to cause much problem for now.

At some point in the future *waves hands ambiguously*, we should actually hammer out all the inconsistencies across the ecosystem. Alas, there are only so many hours in the day.

- ci/39.yaml
- ci/310.yaml
- ci/310-BASE.yaml
- ci/310-DEV.yaml
- ci/311.yaml
include:
- environment-file: ci/310.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Should be testing against 311 for the macOS and Windows environments.

@knaaptime
Copy link
Member Author

LOL I can read this as "dang it, James".

not even close. this is exactly why i tag you for review! :)

@jGaboardi jGaboardi mentioned this pull request Oct 7, 2023
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