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

github actions CI maintainence #2669

Merged

Conversation

JacksonBurns
Copy link
Contributor

  • increment miniconda setup to v3 from v2
  • set macos build runner to use intel macs rather than m1, since we don't have binaries for m1
  • switch download artifact step to use the official github one, which now allows grabbing artifacts from other runs

Resolves #2611

See #2667 for more history

@JacksonBurns
Copy link
Contributor Author

JacksonBurns commented Jun 7, 2024

This should fail at the 'download stable regression results' step, complaining that it could not find the artifact, possibly specifying that the reason it couldn't find it is because it was upload with a previous version of the upload artifact action (see: https://github.com/actions/toolkit/blob/main/packages/artifact/docs/faq.md#which-versions-of-the-artifacts-packages-are-compatible). That would be resolved by ignoring this failure and merging anyway, then letting the CI run on main.

Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

Looks good - if it works as expected.
One suggested change.
Do we need to do anything to RMG-database also?

@@ -215,23 +215,24 @@ jobs:
run: mkdir stable_regression_results

# Retrieve Stable Results for reference
# Will need to use this -> https://github.com/dawidd6/action-download-artifact
- name : ChatGPT said this would work - Get most recent CI run ID
Copy link
Member

Choose a reason for hiding this comment

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

I suggest remove the chatGPT bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha forgot i had that in there, removing now

@JacksonBurns
Copy link
Contributor Author

JacksonBurns commented Jun 7, 2024

Do we need to do anything to RMG-database also?

yes! thanks for the reminder. will do this now

 - increment miniconda setup to v3 from v2
 - set macos build runner to use intel macs rather than m1, since we don't have binaries for m1
 - switch download artifact step to use the official github one, which now allows grabbing artifacts from other runs

Resolves ReactionMechanismGenerator#2611

See ReactionMechanismGenerator#2667 for more history
asdf
JacksonBurns added a commit to JacksonBurns/RMG-actually-a-database that referenced this pull request Jun 7, 2024
 - copy the download logic from RMG-Py (ReactionMechanismGenerator/RMG-Py#2669) that updates the version of download-artifact
 - increment setup-miniconda
@JacksonBurns
Copy link
Contributor Author

@rwest could you take a look at ReactionMechanismGenerator/RMG-database#652? It's the database half of this PR. Both should 'fail' in the same way.

Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

This looks fine to me. If it works as intended then great!

@JacksonBurns JacksonBurns merged commit 9749453 into ReactionMechanismGenerator:main Jun 7, 2024
3 of 4 checks passed
@JacksonBurns JacksonBurns deleted the fix/ci_downloadv4 branch June 7, 2024 16:25
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.

Change CI to use download-artifact@v4
2 participants