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: add vegafusion in ci checks #1228

Merged
merged 13 commits into from
Nov 2, 2024

Conversation

EdAbati
Copy link
Collaborator

@EdAbati EdAbati commented Oct 19, 2024

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

@EdAbati EdAbati marked this pull request as ready for review October 25, 2024 10:13
@MarcoGorelli
Copy link
Member

thanks @EdAbati - this is awesome, but " Successful in 27m " is probably too much for every commit 😄

perhaps we can only trigger this one if we apply some label, so then we can just trigger this before releases?

@EdAbati
Copy link
Collaborator Author

EdAbati commented Oct 25, 2024

Ok CI is green, but it takes quite a lot. Using caching, the build time decreases but pytest takes ~24m (AFAIK similar time as in the vegafusion CI: https://github.com/vega/vegafusion/actions/runs/11419942913/job/31775120908)

Do you know any tricks to speed this up a bit?

Otherwise I would suggest to treat all these downstream libraries tests as "integration tests" and not run them at every push. For example, we could run them just before merging a PR. Is it by using auto_merge_enabled here? (Never done this before though)

Thoughts? @MarcoGorelli @FBruzzesi

EDIT: Marco typed faster than me :D

@EdAbati
Copy link
Collaborator Author

EdAbati commented Oct 25, 2024

perhaps we can only trigger this one if we apply some label, so then we can just trigger this before releases?

Happy to do that. I don't have strong opinions on what should trigger this or when to test these.

Should we run these test before release and triggered by new tag?

Comment on lines +3 to +5
on:
workflow_call:
workflow_dispatch:
Copy link
Collaborator Author

@EdAbati EdAbati Oct 27, 2024

Choose a reason for hiding this comment

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

not sure if this is what I meant, but now this can be triggered:

  • by other workflows (i.e. in the 1st step of the publish workflow here )
  • manually

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

let's give this a go - thanks @EdAbati !

@MarcoGorelli MarcoGorelli merged commit abd9d4a into narwhals-dev:main Nov 2, 2024
20 checks passed
@EdAbati EdAbati deleted the add-vegafusion-ci branch November 2, 2024 22:14
@MarcoGorelli
Copy link
Member

@EdAbati is it possible to have most downstream tests run on every commit, but only the slow ones (for now, just vegafusion) only run before releases?

@EdAbati
Copy link
Collaborator Author

EdAbati commented Nov 3, 2024

is it possible to have most downstream tests run on every commit

I will have a look today 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add vegafusion to downstream ci tests
2 participants