-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
PR: Add manycells and manyoutputs benchmarks on CI and generate artifacts #24
Conversation
9a99cfe
to
37c6cd2
Compare
This is awesome @goanpeca . How can we access the uploaded artifacts?
|
@goanpeca A nice addition would be to have the generated notebook (in the data folder) as downloadable artifacts. I am happy to merge we this as-is, but I guess it should not be difficult to enroll that feature before. |
Rigth!, let me add that |
🤦 you are right! |
@@ -0,0 +1,89 @@ | |||
# Local imports | |||
import json |
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.
@echarles is there a way to not repeat this file on the repo src and on this subpackage?
I tried adding an extra include on the root package but they need to be under the same root folder.
I could not bundle this file and copy it on CI maybe?
Just to avoid having two files falling out of sync
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.
I only see one compare.py
in this PR. What do I miss 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.
The ts file on /packages/compare/src/com..ts. Is a 1-1 cooy of yhe same file in /src/
I also could not get the vl2png thinguie to work 🤷 , but I guess we can enable that later. |
Also, the naming convention is:
Where parameters are the configurable test parameters for |
later is very good. |
Sounds good to me. A new page on the doc site would be great to publish these details for everyone, eg on |
Great idea. Will do! |
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.
@goanpeca Thank you so much for this impressive and very useful work. It is working fine and we can iterate with other PR based on opened issues. Merging.
Add CI to run tests and upload artifacts.
Added metadata on JSON
example:
f7b7ee7-manyCells-manyOutputs-5-0.9-100.json