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

Update deprecated GitHub Actions and add Python versions #408

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

martinrieder
Copy link
Contributor

@martinrieder martinrieder commented Jul 14, 2024

Running 6 different Python versions (3.7 to 3.12) in parallel now.
NOTE: This is in conflict with #309, but can be resolved easily.

GitHub Actions require an update:

  • actions/upload-artifact@v3 is scheduled for deprecation on November 30, 2024.
  • Similarly, v1/v2 are scheduled for deprecation on June 30, 2024.
  • Updating this comes with a breaking change in upload-artifact@v4:
    Uploading to the same named Artifact multiple times.

Due to how Artifacts are created in this new version, it is no longer possible to upload to the same named Artifact multiple times. You must either split the uploads into multiple Artifacts with different names, or only upload once. Otherwise you will encounter an error.

The artifact .zip files therefore have the python version added to their name.

actions/upload-artifact@v3 is scheduled for deprecation on November 30, 2024. Learn more. Similarly, v1/v2 are scheduled for deprecation on June 30, 2024. Please update your workflow to use v4 of the artifact actions.
Breaking change in upload-artifact@v4:
Uploading to the same named Artifact multiple times.

Due to how Artifacts are created in this new version, it is no longer possible to upload to the same named Artifact multiple times. You must either split the uploads into multiple Artifacts with different names, or only upload once. Otherwise you will encounter an error.
@martinrieder
Copy link
Contributor Author

Additionally, I suggest adding the following action: Compare build artifacts from two different commits via github actions

@martinrieder
Copy link
Contributor Author

Note that I have enable PyLint as an additional GH action in my personal repo:
https://github.com/martinrieder/WireViz/blob/master/.github/workflows/pylint.yml

It reports some errors on the current code base that might need fixing...

************* Module src.wireviz.DataClasses
src/wireviz/DataClasses.py:306:23: E1101: Instance of 'float' has no 'split' member (no-member)
src/wireviz/DataClasses.py:306:23: E1101: Instance of 'int' has no 'split' member (no-member)
************* Module src.wireviz.Harness
src/wireviz/Harness.py:371:131: E1[10](https://github.com/martinrieder/WireViz/actions/runs/9930235008/job/27428731076#step:6:11)1: Module 'wireviz.wv_colors' has no 'default_color' member (no-member)
************* Module src.wireviz.wv_cli
src/wireviz/wv_cli.py:153:4: E[11](https://github.com/martinrieder/WireViz/actions/runs/9930235008/job/27428731076#step:6:12)20: No value for argument 'file' in function call (no-value-for-parameter)
src/wireviz/wv_cli.py:153:4: E1120: No value for argument 'format' in function call (no-value-for-parameter)
src/wireviz/wv_cli.py:153:4: E1[12](https://github.com/martinrieder/WireViz/actions/runs/9930235008/job/27428731076#step:6:13)0: No value for argument 'prepend' in function call (no-value-for-parameter)
src/wireviz/wv_cli.py:153:4: E1120: No value for argument 'output_dir' in function call (no-value-for-parameter)
src/wireviz/wv_cli.py:153:4: E1120: No value for argument 'output_name' in function call (no-value-for-parameter)
src/wireviz/wv_cli.py:[15](https://github.com/martinrieder/WireViz/actions/runs/9930235008/job/27428731076#step:6:16)3:4: E1120: No value for argument 'version' in function call (no-value-for-parameter)

If you think that it is useful to add it here as well, then let me know!

@kvid
Copy link
Collaborator

kvid commented Jul 24, 2024

I don't have enough experience with pylint to make well-founded comments about its usefulness for our project. In general, I like tools that warn about possible code errors, but if the number of false positives and false negatives are greater than the true findings, then I'm not sure.

It seems pylint has several reported issues about false positives when the code check isinstance(v, type) before accessing v.attr, and that is a frequent code pattern in the WireViz source code. The two errors from DataClasses.py in your report above are such false positives.

  • The error from Harness.py seams to be a real misspelling error that needs fixing.
  • The errors from wv_cli.py look real, but why does the ordinary CLI use cases work just fine then? There might be some click "magic" involved that I don't understand.

@martinrieder
Copy link
Contributor Author

martinrieder commented Jul 25, 2024

See an interesting article on this topic

TL;DR Recommendation:

  • Type checker: Mypy
  • Error linter: Pyflakes
  • Packaging linter: Pyroma
  • Code formatter: Black
  • Docstring linter/formatter: docformatter
  • Complexity analyzer: Radon

Further reading and a cheat sheet

Personally, I am relying on VScode's Pyright and Pylance, though that is partially closed source, so it is only a conditional recommendation. See DetachHead/basedpyright for details and alternatives.

@amotl
Copy link
Member

amotl commented Sep 18, 2024

Hi. On linting and code formatting concerns, I dearly recommend to use Ruff these days. I am sure you will never look back.

@kvid
Copy link
Collaborator

kvid commented Sep 29, 2024

  • We need to apply such a change ASAP to avoid the GitHub Actions error for each push to the PR branch, that I experience now:
    "This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v2."

  • I want to squash merge this into master as it is, but not yet create a new release. IMHO, that should be OK, because no files used by running code is changed, but please argue against me or suggest alternatives.

  • Adjusting which Python versions to test can be done later in PR Python version update #309. We can keep the 6 tests present here now, to gain some experience.

  • Adding any additional GH actions can be moved to a separate PR.

  • dev and branches in open PRs can then either be rebased on top of the new master or cherry-pick the squash commit to enable using the updated workflow actions.

Do you agree, @martinrieder, @formatc1702, @amotl, and any others with an opinion?

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 5, 2024

I have very limited experience with GH Actions, linters, etc. but here are my main points:

  • We need to apply such a change ASAP to avoid the GitHub Actions error for each push to the PR branch, that I experience now:
    "This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v2."

  • I want to squash merge this into master as it is, but not yet create a new release. IMHO, that should be OK, because no files used by running code is changed, but please argue against me or suggest alternatives.

I see no problem here, go ahead 👍

  • dev and branches in open PRs can then either be rebased on top of the new master or cherry-pick the squash commit to enable using the updated workflow actions.

Cherry-picking should be enough to avoid rebase hell ;)

TL;DR Recommendation:
* Type checker: Mypy
* Error linter: Pyflakes
* Packaging linter: Pyroma
* Code formatter: Black
* Docstring linter/formatter: docformatter
* Complexity analyzer: Radon

Are you suggesting use of these tools should be enforced for all contributors? Would this increase the difficulty for new contributors? As someone who has not used these tools (beside black) I can't gauge the usefulness/necessity here.

@kvid
Copy link
Collaborator

kvid commented Oct 5, 2024

@formatc1702 - Thank's for responding!

I have very limited experience with GH Actions, linters, etc.

The same goes for me, I'm afraid (except isort & black).

but here are my main points:

  • We need to apply such a change ASAP to avoid the GitHub Actions error for each push to the PR branch, that I experience now:
    "This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v2."
  • I want to squash merge this into master as it is, but not yet create a new release. IMHO, that should be OK, because no files used by running code is changed, but please argue against me or suggest alternatives.

I see no problem here, go ahead 👍

Thank's - I'll do this within in a couple of days, unless someone else present a better alternative before then.

  • dev and branches in open PRs can then either be rebased on top of the new master or cherry-pick the squash commit to enable using the updated workflow actions.

Cherry-picking should be enough to avoid rebase hell ;)

I see your point, but I think we can let the PR authors decide for each case. In some cases, getting also all the latest bugfixes might be worth handling a few conflicts.

TL;DR Recommendation:

  • Type checker: Mypy
  • Error linter: Pyflakes
  • Packaging linter: Pyroma
  • Code formatter: Black
  • Docstring linter/formatter: docformatter
  • Complexity analyzer: Radon

Are you suggesting use of these tools should be enforced for all contributors? Would this increase the difficulty for new contributors? As someone who has not used these tools (beside black) I can't gauge the usefulness/necessity here.

@martinrieder must answer for his suggestions himself, but I suggest moving any additional actions to new PR(s).

@kvid kvid merged commit e8c482e into wireviz:master Oct 14, 2024
6 checks passed
kvid pushed a commit to kvid/WireViz that referenced this pull request Oct 14, 2024
Running 6 different Python versions (3.7 to 3.12) in parallel now.
NOTE: This is in conflict with wireviz#309, but can be resolved easily in a later PR.

GitHub Actions require an update:
- actions/upload-artifact@v3 is scheduled for deprecation on November
30, 2024.
- Similarly, v1/v2 are scheduled for deprecation on June 30, 2024. 
- Updating this comes with a breaking change in upload-artifact@v4:

Uploading to the same named Artifact multiple times.

Due to how Artifacts are created in this new version, it is no longer
possible to upload to the same named Artifact multiple times. You must
either split the uploads into multiple Artifacts with different names,
or only upload once. Otherwise you will encounter an error.

The artifact .zip files therefore have the python version added to
their name.
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.

4 participants