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

ci: Remove unnecessary Python setup step #285

Merged
merged 1 commit into from
May 23, 2024
Merged

Conversation

PGijsbers
Copy link
Collaborator

Reference Issues/PRs

I could've opened a separate issue, but I figured a PR makes it easier to view my suggestion.

What does this implement/fix? Explain your changes.

When looking at this workflow I noticed that there seems to be an extraneous Python setup step in the citizen bump job. As far as I can tell, it serves no purpose. In my work-in-progress workflow I left it out without issue.

Minimal Example / How should this PR be tested?

Run the workflow (on a fork where it is not a trusted PyPI publisher...). I tried it here, but unfortunately the docs step already seems to be failing, so it won't show a successful bump. So that step would need to be resolved first, but it's unrelated to this PR. After that, you should see that indeed the Python setup step is not necessary to execute the bump job and you can release 23 seconds faster. Nifty.

Any other comments?

Made the change from the browser, so it did bypass pre-commit. I assume there is a pre-commit CI running so that if there's a problem it'll be detected before a merge anyway.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eddiebergman
Copy link
Contributor

Oh very nice, thanks for this! Always nice to shave off time when watching the release workflow in anticipation ;)

I looked at the docs build step and I am clueless as to why that is failing?
I've ran it again on master so hopefully it fails there (hopefully meaning it's reproducbile), otherwise this is a mystery...
https://github.com/automl/amltk/actions/runs/9214528781

I will merge it anywho tomorrow morning, I just want to see if there's some actionable signal first.


Side note, what are you using it for? If you need any help regarding mkdocs/mike, or the requirements for this let me know! Something which I'm not sure if documented as that it relies on commitizen and explicitly no one manually updating versions.

@eddiebergman
Copy link
Contributor

eddiebergman commented May 23, 2024

Merging, lovely PR <3

I found my culprit to be markdown-exec. I've had to monkey patch this a few times for controlling example rendering in docs with env variables as well as allow for multiprocessing... If you come across any other solutions to runnable markdown for mkdocs, I would be grateful for knowing!

The Python setup isn't actually used in the remainder of the jobs.
@eddiebergman eddiebergman merged commit c3eb4ac into automl:main May 23, 2024
3 of 4 checks passed
@PGijsbers
Copy link
Collaborator Author

Merging, lovely PR <3

Thanks!

Side note, what are you using it for?

Right now, I want to use it in aiondemand: a Python wrapper for the AI-on-Demand REST API. But realistically I want all "my" Python projects to have an easy build/release workflow eventually.

If you need any help regarding mkdocs/mike, or the requirements for this let me know!

Thanks! So far not really encountering any issues, the projects' documentation and your working reference workflow are good resources :)

If you come across any other solutions to runnable markdown for mkdocs, I would be grateful for knowing!

Not aware of any.

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