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

linting + CI #96

Merged
merged 17 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
name: Python cryoet_data_portal package unit tests
name: Python Client Tests

on:
push:
branches:
- main
paths:
- 'client/python/**'
- '.github/workflows/**'
pull_request:
branches:
- '**'
paths:
- 'client/python/**'
- '.github/workflows/**' # Re-run if a workflow is modified - useful to test workflow changes in PRs
push:
branches: [main]
- '.github/workflows/**'

jobs:
unit_tests_python_client:
test-client-python:
name: Test Python Client
runs-on: ubuntu-latest

strategy:
Expand All @@ -18,48 +25,61 @@ jobs:
python-version: ['3.7', '3.8', '3.9', '3.10']

steps:
- uses: actions/checkout@v3
- name: Checkout Repo
uses: actions/checkout@v3

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}

- name: Install dependencies
run: |
python -m pip install -U pip setuptools wheel
pip install -r ./client/python/cryoet_data_portal/scripts/requirements-dev.txt
pip install -e ./client/python/cryoet_data_portal/

- name: Test with pytest (API)
run: |
PYTHONPATH=. coverage run --parallel-mode -m pytest -v -rP --durations=20 ./client/python/cryoet_data_portal/tests/
- uses: actions/upload-artifact@v3

- name: Upload Coverage
uses: actions/upload-artifact@v3
with:
name: coverage
path: ./.coverage*
retention-days: 3

submit-codecoverage:
needs:
- unit_tests_python_client
- test-client-python
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v2
- name: Checkout Repo
uses: actions/checkout@v3
with:
fetch-depth: 0
- uses: actions/download-artifact@v3

- name: Download Coverage
uses: actions/download-artifact@v3
with:
name: coverage
path: .
- uses: actions/setup-python@v4

- name: Setup Python
uses: actions/setup-python@v4
with:
python-version: '3.10'
- name: install requirements

- name: Install Requirements
run: |
pip install coverage
- name: coverage report
- name: Generate Coverage Report
run: |
coverage combine
coverage xml
- name: Upload coverage to Codecov

- name: Upload Coverage to Codecov
uses: codecov/codecov-action@v3
with:
token: ${{ secrets.CODECOV_TOKEN }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/docsite-build-deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
with:
python-version: "3.10"

- name: Install Python deps 🔧
- name: Install Python deps 🔧
run: |
python -m pip install -U pip setuptools wheel
pip install -r ./client/python/cryoet_data_portal/scripts/requirements-dev.txt
Expand Down
69 changes: 69 additions & 0 deletions .github/workflows/formatting.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
name: Pre-Commit Formatting

on:
pull_request:
paths:
- 'client/**'
- 'frontend/**'
- 'backend/**'
- '.github/workflows/**' # Re-run if a workflow is modified - useful to test workflow changes in PRs
push:
branches:
- main

jobs:
pre-commit-checks:
name: pre-commit checks
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: '3.9'
Comment on lines +20 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically not, no

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker: We could remove it.


- uses: dorny/paths-filter@v2
id: changes
with:
filters: |
frontend:
- 'frontend/**'

- uses: actions/setup-node@v3
if: steps.changes.outputs.frontend == 'true'
with:
node-version: 20.8.x

- name: Install PNPM
if: steps.changes.outputs.frontend == 'true'
uses: pnpm/action-setup@v2
with:
version: 8.9.0
run_install: false

- name: Get pnpm store directory
if: steps.changes.outputs.frontend == 'true'
shell: bash
run: |
echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV

- name: Setup pnpm cache
uses: actions/cache@v3
if: steps.changes.outputs.frontend == 'true'
with:
path: ${{ env.STORE_PATH }}
key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-pnpm-store-

- name: Install dependencies with PNPM
if: steps.changes.outputs.frontend == 'true'
run: pnpm install --frozen-lockfile
working-directory: frontend

- name: Build GraphQL Types
if: steps.changes.outputs.frontend == 'true'
run: pnpm build
working-directory: frontend/packages/data-portal

- name: run pre-commit
uses: pre-commit/[email protected]
79 changes: 79 additions & 0 deletions .github/workflows/frontend-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Workflow for running several tests for PRs and pushes to the main branch for
# data portal frontend code.

name: Frontend Tests

on:
push:
branches:
- main
paths:
- "frontend/**"
pull_request:
branches:
- "**"
paths:
- "frontend/**"

defaults:
run:
working-directory: frontend/

jobs:
# Runs several tests on the frontend codebase to check for code style,
# formatting, best practices, and run unit/integration tests.
tests:
name: ${{ matrix.name }}
runs-on: ubuntu-latest

steps:
- name: Checkout Repo
uses: actions/checkout@v3

- name: Setup Node
uses: actions/setup-node@v3
with:
node-version: 20.8.x

- name: Install PNPM
uses: pnpm/action-setup@v2
with:
version: 8.9.0
run_install: false

- name: Get pnpm store directory
shell: bash
run: |
echo "STORE_PATH=$(pnpm store path --silent)" >> $GITHUB_ENV

- name: Setup pnpm cache
uses: actions/cache@v3
with:
path: ${{ env.STORE_PATH }}
key: ${{ runner.os }}-pnpm-store-${{ hashFiles('**/pnpm-lock.yaml') }}
restore-keys: |
${{ runner.os }}-pnpm-store-

- name: Install dependencies with PNPM
run: pnpm install --frozen-lockfile
working-directory: frontend

- name: Build GraphQL Types
run: pnpm build
working-directory: frontend/packages/data-portal

- name: ${{ matrix.name }}
run: ${{ matrix.run }}

# Run linters and tests as matrix strategies so that they:
# 1. Show up as separate GitHub checks.
# 2. So we can reuse the Node.js and PNPM installation steps
strategy:
fail-fast: false
matrix:
include:
- name: jest
run: pnpm -r test

- name: tsc
run: pnpm -r type-check
2 changes: 1 addition & 1 deletion .github/workflows/prod-deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ on:
push:
branches:
- "main"

permissions:
id-token: write
contents: read
Expand Down
21 changes: 0 additions & 21 deletions .github/workflows/py-formatting.yml

This file was deleted.

3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ __pypackages__/
celerybeat-schedule
celerybeat.pid

# TypeScript stuff
tsconfig.tsbuildinfo

# SageMath parsed files
*.sage.py

Expand Down
33 changes: 33 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
repos:
- repo: https://github.com/psf/black-pre-commit-mirror
rev: 23.9.1
hooks:
- id: black
files: backend|client/python
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.0.292
hooks:
- id: ruff
files: backend|client/python
args:
- --fix
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.5.0
hooks:
- id: check-toml
- id: check-yaml
exclude: frontend # covered by prettier
- id: check-json
exclude: frontend # covered by prettier
- id: check-merge-conflict
- id: end-of-file-fixer
- id: trailing-whitespace
- repo: local
hooks:
- id: pnpm-lint
name: pnpm lint
entry: pnpm -r --parallel lint:fix
language: system
types_or: [javascript, jsx, ts, tsx]
files: frontend
pass_filenames: false
2 changes: 1 addition & 1 deletion client/python/cryoet_data_portal/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# CryoET Portal api client

The `cryoet_data_portal` package provides a python client to facilitate the use of the Cryo-Electron Tomography Portal. For more information about the API and the project visit the [chanzuckerberg/cryoet-data-portal GitHub repo](https://github.com/chanzuckerberg/cryoet-data-portal/).
The `cryoet_data_portal` package provides a python client to facilitate the use of the Cryo-Electron Tomography Portal. For more information about the API and the project visit the [chanzuckerberg/cryoet-data-portal GitHub repo](https://github.com/chanzuckerberg/cryoet-data-portal/).


## For More Help
Expand Down
13 changes: 0 additions & 13 deletions client/python/cryoet_data_portal/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ exclude = ["tests*"] # exclude packages matching these glob patterns (empty by
[tool.setuptools_scm]
root = "../../.."

[tool.black]
line-length = 120
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be good to retain, as the default line limit of 80 is quite short.

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 can revert this if you desire

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker: Yes, please, if it isn't too much trouble.

target_version = ['py39']

[tool.mypy]
show_error_codes = true
ignore_missing_imports = true
Expand All @@ -60,12 +56,3 @@ strict = true
markers = [
"expensive: too expensive to run regularly or in CI",
]

[tool.ruff]
select = ["E", "F", "B", "I"]
ignore = ["E501", "E402", "C408", ]
line-length = 120
target-version = "py39"

[tool.ruff.isort]
known-first-party =["cryoet_data_portal"]
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
-r ../../../../py-dev-requirements.txt
numpy
pytest
requests-mock
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@
import importlib_metadata as metadata # type: ignore[no-redef]

from ._client import Client
from ._models import Annotation, AnnotationAuthor, Dataset, DatasetAuthor, DatasetFunding, Run, TiltSeries, Tomogram, TomogramVoxelSpacing
from ._models import (
Annotation,
AnnotationAuthor,
Dataset,
DatasetAuthor,
DatasetFunding,
Run,
TiltSeries,
Tomogram,
TomogramVoxelSpacing,
)

try:
__version__ = metadata.version("cryoet_data_portal")
Expand Down
Loading
Loading