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

Andrew/new kaleido #7241

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Andrew/new kaleido #7241

wants to merge 2 commits into from

Conversation

ayjayt
Copy link
Contributor

@ayjayt ayjayt commented Oct 21, 2024

This pr will try to run tests with the new kaledio

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

@ayjayt Thanks very much for this PR as well as your outstanding work on the Kaleido project. 🏆 🥇
There is a error at https://app.circleci.com/pipelines/github/plotly/plotly.js/11581/workflows/73a8d28d-0e75-42a6-b4ad-ff097d9f5459/jobs/256234/parallel-runs/0/steps/0-103

Noting that on plotly.js side we switched from webpack to esbuild which now generates ES6 bundles instead of ES5 bundles, on plotly.py we need to switch from requirejs to native ES6 import. See plotly/plotly.py#4763.

If you thought this might be the cause of the error, I suggest you rebase your work on top of the release-v2.35.3 branch which still uses webpack and target the release-v2.35.3 instead of master.

Thank you!

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

@gvwilson gvwilson requested a review from archmoj October 21, 2024 15:11
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Oct 21, 2024
@ayjayt
Copy link
Contributor Author

ayjayt commented Oct 21, 2024

Hey thanks @archmoj @gvwilson

I appreciate the help

Just ignore the PR, it won't be merged, I'm just telling CI to use pre-release stuff for testing. But I do appreciate the input! Thanks!

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

Hey thanks @archmoj @gvwilson

I appreciate the help

Just ignore the PR, it won't be merged, I'm just telling CI to use pre-release stuff for testing. But I do appreciate the input! Thanks!

@ayjayt Just curious, could you possibly test this by releasing an RC of Kaleido?
If so please coordinate that with @gvwilson first.
Thank you!

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

Alternatively you may try npm install plotly.js locally and then cd ploltly.js and npm install and then npm run baseline.

@ayjayt
Copy link
Contributor Author

ayjayt commented Oct 21, 2024

@archmoj

  1. i'm under the impression we're avoiding using circleci right now based on asking me to test locally-

  2. If circleci is using an old cache of pypi that's why we can't install choreographer. I can't test that except through circleci.

  3. Just curious, could you possibly test this by releasing an RC of Kaleido?

We'd still need to change the pip commands here to use that, I think, right, we'd have to add --pre-release or something?

  1. Running locally has tons of errors, notably that I don't have credentials:
  • npm install plotly.js hangs
  • npm run baseline just ends at "Please wait" for me..
  • python3 make_baseline.py = doesn't work because I don't have credentials needed
  • npm run build has a canvas error

Error: Not implemented: HTMLCanvasElement.prototype.getContext (without installing the canvas npm package)

So I'll just wait for advice before doing further testing...

If its a matter of cose, I can add holds to all circleci process except the ones I want to test?

@archmoj
Copy link
Contributor

archmoj commented Oct 21, 2024

After installing plotly.js, you need to run npm run pretest then python3 make_baseline.py =.
Hope that fix the problem for you.
Please let me know.

@ayjayt
Copy link
Contributor Author

ayjayt commented Oct 21, 2024

Ah yes, now python3 make_baseline.py and npm run baseline seem to work, thanks

@archmoj
Copy link
Contributor

archmoj commented Oct 22, 2024

If you noticed different fonts on the baselines, on GNU+Linux you may consider installing them using something like:

# install required fonts
sudo apt-get install fonts-liberation2 fonts-open-sans fonts-noto-cjk fonts-noto-color-emoji && \
sudo python3 .circleci/download_google_fonts.py && \
sudo cp -r .circleci/fonts/ /usr/share/ && \
sudo fc-cache -f && \

@archmoj
Copy link
Contributor

archmoj commented Oct 29, 2024

@ayjayt Were you able to generate the baselines locally?

@ayjayt
Copy link
Contributor Author

ayjayt commented Oct 29, 2024

@ayjayt Were you able to generate the baselines locally?

Your instructions worked and the commands executed without error, I did not visually inspect all the graphs. (thanks for all that)

I want to test against the pre-release here before pushing kaleido to main and pypi so not to break all of your testing on that push. Some issues are circle-ci specific. But of course, its up to you.

@archmoj
Copy link
Contributor

archmoj commented Oct 29, 2024

What's the CirclCI issue?

@gvwilson gvwilson removed the request for review from archmoj November 13, 2024 14:46
@gvwilson
Copy link
Contributor

@ayjayt can we close this one?

@ayjayt
Copy link
Contributor Author

ayjayt commented Nov 13, 2024

If you want to test your CI systems w/ the new kaleido, you will need a PR.

Previously, your CI systems had some issues related to missing dependencies.

Its up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants