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

Clean up CI/CD workflows #1278

Merged
merged 19 commits into from
Jan 12, 2023

Conversation

VeckoTheGecko
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko commented Jan 7, 2023

Rewrote workflows to be more concise, leveraging the setup-miniconda GitHub Action.

Changes:

  • Updated miniconda install to use setup-miniconda
    • Removed now obsolete scripts
  • Separated linting into its own file
  • Updated workflow triggers for linting and testing (adding test-me/* branch namespace for future testing of CI/CD)
  • Generate HTML reports for tests, and save as artifacts

@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Jan 7, 2023

In addition to the failing test in master, the mac tests aren't passing for JIT compilation due to the removal of this line:
https://github.com/OceanParcels/parcels/blob/56e988ae0f627ec8994c656f5f4bab71154a2e47/.github/scripts/osx_unittest.sh#L5

This can be included in the action by checking the platform, which I'm happy to implement in the coming days.

@VeckoTheGecko VeckoTheGecko marked this pull request as draft January 8, 2023 10:54
@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review January 8, 2023 12:45
@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Jan 8, 2023

Testing is now working on mac. Ready for feedback (sorry for the noise)

Happy to wait for master to pass tests before merging this.

@erikvansebille
Copy link
Member

erikvansebille commented Jan 9, 2023

Thanks @VeckoTheGecko, for this great improvement in the testing functionality!

A few quick comments:

  • The integration-tests doesn't seem to work yet?
  • Are the three warnings at e.g. Unittesting on ubuntu with python latest
    Node.js 12 actions are deprecated. For more information see: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/. Please update the following actions to use Node.js 16: actions/upload-artifact@v2 something to worry about? Something that can be fixed?
  • There still seems to be a lot of code duplication between the unit-tests and the integration-tests; can that not be streamlined in some way?
  • I like the report in html a lot! Didn't know that was possible. But any way to not have it be zipped before downloading? (so that it's even simpler to open it)

@VeckoTheGecko
Copy link
Contributor Author

Not sure what happened there. Seems like they were skipped for some reason. We can check again on the next run.

  • Are the three warnings ...

Easy fix. Thanks for raising that

  • There still seems to be a lot of code duplication between the unit-tests and the integration-tests; can that not be streamlined in some way?

Actually, yes! Just went down a bit of a rabbit hole into composite actions, and updated accordingly. It should be working, let me know what you think.

  • I like the report in html a lot! Didn't know that was possible. But any way to not have it be zipped before downloading? (so that it's even simpler to open it)

All artifacts are zipped, so you can't just download plain HTML (or "open in new tab"). I know for coverage there are platforms to upload coverage reports. There could be similar platforms for testing, but I think it might not be worth it. Runs will hopefully only fail occasionally (and when they do, downloading the report isn't much overhead).

Speaking of coverage, I'll implement it sometime after this PR has merged.


On another note, I think the Windows builds are all failing without logs (even on PR #1273, prior to the workflow changes). Would it be worth disabling Windows builds? I'm not sure what the problem is (relevant issue)

@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Jan 9, 2023

Also just renamed the workflow files and workflow names ot be more concise
(happy to reverty those renamings if thats preferable)

@erikvansebille
Copy link
Member

I've now fixed the vonmises issue in PR #1279, but don't get green ticks with Windows because of a strange error

D:\a\parcels\parcels>call activate parcels 
[446](https://github.com/OceanParcels/parcels/actions/runs/3879215870/jobs/6624344113#step:3:447)

[447](https://github.com/OceanParcels/parcels/actions/runs/3879215870/jobs/6624344113#step:3:448)
EnvironmentNameNotFound: Could not find conda environment: parcels
[448](https://github.com/OceanParcels/parcels/actions/runs/3879215870/jobs/6624344113#step:3:449)
You can list all discoverable environments with `conda info --envs`.

@VeckoTheGecko, can you try to merge #1279 into this one, to see if it works then?

@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented Jan 11, 2023

@erikvansebille Just merged, hopefully those tests pass now 🤞

I'm going to dial back my activity on this PR in favour of other areas of the codebase (debugging these workflows is doing my head in).

@VeckoTheGecko
Copy link
Contributor Author

Looks like the tests are still failing on Windows, and I don't know if this will be a simple fix. If the macos/linux tests pass, I think we should:

  1. revert memory swap fix
  2. remove Windows from testing matrix
  3. create new issue for Windows CI/CD
  4. merge this PR if everything else is in order

@erikvansebille
Copy link
Member

I've now fixed it in #1274 (with the solution at 36cdbd3); can you implement that too here? That should give us green ticks for CI?

@VeckoTheGecko
Copy link
Contributor Author

Tests should pass now. If all is in order, feel free to squash and merge this (as most commits here are just noise).

Co-authored-by: Vecko <36369090+VeckoTheGecko@users.noreply.github.com>
@erikvansebille erikvansebille merged commit f72c458 into OceanParcels:master Jan 12, 2023
@VeckoTheGecko VeckoTheGecko deleted the feat/clean-ci branch January 25, 2023 17:54
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.

2 participants