Conversation

ayjayt

This pr will try to run tests with the new kaledio

@archmoj

@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//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

@gvwilsongvwilson requested a review from archmoj October 21, 2024 15:11
@gvwilsongvwilson added featuresomething newP1needed for current cyclelabels Oct 21, 2024
@ayjayt

Hey thanks @archmoj @gvwilson

I appreciate the help

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

@archmoj

Hey thanks @archmoj @gvwilson

I appreciate the help

Just ignore the PR, it won't be merged, I'm just telling CI to use 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

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

@ayjayt

@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 -- 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..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

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

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

@archmoj

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

@ayjayt Were you able to generate the baselines locally?

@ayjayt

@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 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

What's the CirclCI issue?

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

@ayjayt can we close this one?

@ayjayt

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 . Already have an account? Sign in to comment
featuresomething newP1needed for current cycle
None yet

Successfully merging this pull request may close these issues.