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

gh-actions: Build and upload all artifacts #222

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

Conversation

JohnAZoidberg
Copy link
Contributor

@JohnAZoidberg JohnAZoidberg commented Sep 23, 2024

Fixes #217

Describe your changes

  • If a dataset was added/changed, morph it to shapes: bullseye heart rectangle star slant_up
  • If a shape was added/changed, morph starting from dataset music
  • Otherwise morph music to bullseye heart rectangle star slant_up

The result is uploaded to the github actions run.
I tried to make it run in parallel but it doesn't seem to really work on github actions.

But if you run the following on your local system, it'll morph all 6 in roughly the same time as a single one.

parallel -j0 data-morph \
     --start-shape music \
     --target-shape {} \
     ::: bullseye heart rectangle star slant_up

Checklist

  • Test cases have been modified/added to cover any code changes.
  • Docstrings have been modified/created for any code changes.
  • All linting and formatting checks pass (see the contributing guidelines for more information).
  • If you added a new dataset or shape, please comment on which datasets worked best for your shape or which shapes worked best for your dataset and provide the GIFs for those here.

@github-actions github-actions bot added the ci/cd Relating to local development or CI/CD label Sep 23, 2024
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.47%. Comparing base (1f7779c) to head (e3f6011).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #222   +/-   ##
=======================================
  Coverage   98.47%   98.47%           
=======================================
  Files          43       43           
  Lines        1839     1839           
  Branches      114      114           
=======================================
  Hits         1811     1811           
  Misses         25       25           
  Partials        3        3           

Signed-off-by: Daniel Schaefer <[email protected]>
@stefmolin stefmolin added this to the 0.3.0 milestone Sep 25, 2024
Copy link
Owner

@stefmolin stefmolin left a comment

Choose a reason for hiding this comment

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

I took a quick look over the files for now. Don't worry about force-pushing as I will squash.

$ python bin/ci.py src/data_morph/shapes/bases/line_collection.py
high_lines h_lines slant_down slant_up v_lines wide_lines x diamond rectangle star
python bin/ci.py src/data_morph/data/starter_shapes/superdatascience.csv
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
python bin/ci.py src/data_morph/data/starter_shapes/superdatascience.csv
$ python bin/ci.py src/data_morph/data/starter_shapes/superdatascience.csv

Comment on lines +15 to +18
from data_morph.shapes.factory import ShapeFactory
from data_morph.data.loader import DataLoader
import sys
from os.path import basename
Copy link
Owner

Choose a reason for hiding this comment

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

I figured out why the linting action didn't run on this file. This PR should also add the bin directory to the check-pr.yml workflow:

on:
pull_request:
paths:
- 'docs/**'
- 'src/**'
- 'tests/**'

Once you add that, there will be things to fix.


jobs:
build:
name: Build with Python ${{ matrix.python-version }} on ${{ matrix.os }}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
name: Build with Python ${{ matrix.python-version }} on ${{ matrix.os }}
name: Run Data Morph on new/altered datasets/shapes

python -m pip install setuptools --upgrade
python -m pip install .
- name: Get all dataset files that have changed
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- name: Get all dataset files that have changed
- name: Get all dataset and shape files that have changed

- name: Get all dataset files that have changed
id: changed-files-yaml
uses: tj-actions/changed-files@v45
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add a comment with the documentation link for this.

pull_request:
paths-ignore:
- 'docs/**'
- 'src/tests/**'
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- 'src/tests/**'
- 'tests/**'

Comment on lines +13 to +15
paths-ignore:
- 'docs/**'
- 'src/tests/**'
Copy link
Owner

Choose a reason for hiding this comment

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

What about running only if we have changes in src/**?

Comment on lines +83 to +85
data-morph \
--start-shape music \
--target-shape $SHAPE_ARGS \
Copy link
Owner

Choose a reason for hiding this comment

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

Should we use parallel here like the other two?

# For core code changes, we want to do a couple morphs to see if they still look ok
# Only need to run if neither of the previous two morphs ran
- name: Morph shapes
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- name: Morph shapes
- name: Morph shapes with core code changes

Comment on lines +3 to +4
Pass in the filenames that changed and it'll tell you the arguments of datasets and shapes.
See examples below
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Pass in the filenames that changed and it'll tell you the arguments of datasets and shapes.
See examples below
Call this script with the names of files that have changed to get the
datasets and shapes to test with the CLI.
Examples
--------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Relating to local development or CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GitHub Action to generate GIFs for all datasets/shapes when someone adds a new one in a PR
2 participants