-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
add downstream.yml #1555
add downstream.yml #1555
Conversation
This is cool, but having it in every PR is maybe too much? Can we trigger it only on request same as people in Base do with |
Yeah something like invoking via a GH comment, and we automatically do it for release branches? Like when we create a branch for a new breaking release, this should be a sanity check that runs on that branch. |
Has this functionality been rolled into the FluxBot/ModelZookeeper or is it still WIP? |
Add your friendly neighborhood scientists to the list? 😅 |
Co-authored-by: Johnny Chen <johnnychen94@hotmail.com>
Is this good to go? What's the best/fastest way to test it? |
.github/workflows/Downstream.yml
Outdated
julia-version: [1] | ||
os: [ubuntu-latest] | ||
package: | ||
- {user: FluxML, repo: ObjectDetector.jl, group: All} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question, what does group
mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which test group, https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/test/runtests.jl#L18 so you can run a subset of the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a simplified version of https://github.com/JuliaTesting/ReTest.jl
I guess is to tag this PR with "run downstream test" and then push an empty commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on Slack comments, let's merge this now. We can always tweak it if it runs too much or takes too long, etc.
There is a problem with the name. Also, is it actually working? More than here, it is important to have downstream integration in Zygote, which is more fragile |
We need to label this PR as "run downstream test" which I can't seem to do on mobile. That will trigger the workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this works for most of our downstream targets. I am not sure why Transformers or ObjectDetector throw those errors. I recommend we merge this PR and solve those issues, tweak the trigger, etc. in a different PR.
Having running tests is better than nothing.
Add some downstream tests