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: Remove references to archived altair_viewer and altair_saver in ci, docs, and tests. Uninstall anywidget and vl-convert-python during one test run #3419

Merged
merged 15 commits into from
May 18, 2024

Conversation

binste
Copy link
Contributor

@binste binste commented May 12, 2024

Now that both altair_viewer and altair_saver are archived and not compatible with Altair anyway, I'd suggest to remove all references to them to clean up the code base a bit. In this PR, I:

  • removed all references to altair_viewer
  • removed all references to altair_saver except for 1 mention in the docs
  • uninstall anywidget and vl-convert-python as well in the build workflow to make sure we don't introduce hard dependencies. This is not directly related to the rest of the PR but hope it's ok for efficiency reasons. Else, happy to take it out!
  • added a deprecation warning if someone uses the webdriver argument to the .save method as it's not relevant for vl-convert. I didn't want to remove it just now as users might still have it set although they have switched to vl-convert in which case it has just been ignored so far
  • run test_save_html again with inline=True as vl-convert supports it

@binste
Copy link
Contributor Author

binste commented May 12, 2024

Need to wait for #3418 to be resolved before tests can run.

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this cleanup @binste!

@mattijn
Copy link
Contributor

mattijn commented May 15, 2024

#3418 is resolved, but tests are not yet happy. It now raises an anywidget dependency issue, see https://github.com/vega/altair/actions/runs/9090334764/job/24983082085. Probably related to this sentence in OP:

uninstall anywidget and vl-convert-python as well in the build workflow to make sure we don't introduce hard dependencies.

Can you have a look to this @binste. Thanks!

@binste binste changed the title ci: Remove references to archived altair_viewer and altair_saver in ci, docs, and tests ci: Remove references to archived altair_viewer and altair_saver in ci, docs, and tests. Uninstall anywidget and vl-convert-python during one test run May 18, 2024
@binste
Copy link
Contributor Author

binste commented May 18, 2024

Thank you both! I added pytest.skip statements to some more tests which relied on vl-convert or anywidget. Now, the tests can be run with only Altair's required dependencies. Any tests which require optional dependencies are skipped. Merging as it's only CI changes and Jon approved an earlier version but happy to do work in a follow-up PR if anyone has comments.

@binste binste merged commit 81de7c4 into vega:main May 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants