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

feat: Add support for custom (or offline) Mermaid.ink server and support all parameters #8772

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lbux
Copy link
Contributor

@lbux lbux commented Jan 27, 2025

Related Issues

Proposed Changes:

This PR is an attempt at adding support for offline pipeline draw() and show() calls for users in constrained environments where no requests to outside servers are allowed. This PR builds upon #8767 and introduces no breaking changes to regular draw() and show() calls (from what I've tested). Additionally, all parameters supported by Mermaid.ink are supported. The defaults remain as it was before (PNG image, neutral theme)

How did you test it?

Unit and integration tests were added testing different combinations of parameters. No tests were added to test a remote server since there is no way to properly test that in the CI. I tested it myself using my own docker hosted Mermaid server, but further testing would be needed. The original tests were not modified significantly to show the changes are most likely not breaking changes.

Notes for the reviewer

Several notes:

  • When working on adding custom server support, I decided to just go ahead and add extra parameter support. If that isn't needed, we can just keep the server support and scrap everything else.
  • Most of the additional changes are just checks to ensure the parameters passed in by the users are valid. I tried to raise errors for possible common mistakes. If we don't need this many or if we want to add more, please let me know.
  • If we still want to add an integration test for a remote server, I can add it.
  • Given that now allowed for pdf support, it might make sense to rename _to_mermaid_image to something else.
  • This can technically be done with Mermaid-py but after taking a look at the code, it doesn't really make sense to bring in an extra dependency to end up making a request either way.

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@lbux lbux requested review from a team as code owners January 27, 2025 05:30
@lbux lbux requested review from dfokina and Amnah199 and removed request for a team January 27, 2025 05:30
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 12982662256

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 55 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.09%) to 91.241%

Files with Coverage Reduction New Missed Lines %
core/pipeline/draw.py 15 87.29%
core/pipeline/base.py 40 88.64%
Totals Coverage Status
Change from base Build 12948411569: -0.09%
Covered Lines: 8896
Relevant Lines: 9750

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Offline Rendering for pipeline.show()
2 participants