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 plantuml generated diagrams #1509

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

Silvanoc
Copy link
Contributor

@Silvanoc Silvanoc commented Jun 23, 2023

Adding an alternative UML generator. The advantage of this proposal over the existing one (YUML) is that it enables the usage of a service that can be self-hosted and doesn' have any license fees.

The source code is a modified version of the YUML generator.

@Silvanoc Silvanoc force-pushed the add-plantuml-support branch from f30e449 to 163b2d0 Compare June 23, 2023 10:10
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2023

Codecov Report

Merging #1509 (9fab2b9) into main (ed0c618) will increase coverage by 0.15%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main    #1509      +/-   ##
==========================================
+ Coverage   79.95%   80.10%   +0.15%     
==========================================
  Files          93       94       +1     
  Lines        9711     9898     +187     
  Branches     2382     2439      +57     
==========================================
+ Hits         7764     7929     +165     
- Misses       1502     1515      +13     
- Partials      445      454       +9     
Files Changed Coverage Δ
linkml/generators/__init__.py 100.00% <ø> (ø)
linkml/generators/plantumlgen.py 88.23% <88.23%> (ø)

@dalito
Copy link
Member

dalito commented Jun 23, 2023

Nice! Will it be possible to produce puml-files (or just final images)?

@Silvanoc
Copy link
Contributor Author

Nice! Will it be possible to produce puml-files (or just final images)?

@dalito Thanks!

I've implemented it the same way as the yUML generator. If you don't pass the option -d/--directory, then you get the PlantUML code printed out.

@Silvanoc
Copy link
Contributor Author

For backwards compatibility reasons I've kept the gen-markdown option --noyuml, although IMO it should be rather be called --nouml and apply to both PlantUML and yUML.

@sujaypatil96
Copy link
Member

@Silvanoc: thank you for your work on this! I didn't know about the plantuml project before this, but it looks like another tool with UML diagram generation capabilities. Currently, the de facto tool that we're using within the linkml framework to generate/render UML diagrams is mermaid. The only kind of diagrams that we are rendering as part of the web documentation generator (gen-doc/docgen.py) at the moment are class diagrams (granted showing minimal relationship information, simply because we haven't prioritized it enough to improve the level of detail it shows).

I have a couple of questions for you -

  1. Does the plantUML generator you wrote show more comprehensive linking and provide more valuable information than the current mermaid diagrams?
  2. Why not focus on improving the detail in the mermaid diagrams rather than introducing another UML diagramming tool? Is there something lacking in mermaid that plantUML provides?

There's also an ER diagram generator in the framework: https://github.com/linkml/linkml/blob/main/linkml/generators/erdiagramgen.py

@sujaypatil96
Copy link
Member

For backwards compatibility reasons I've kept the gen-markdown option --noyuml, although IMO it should be rather be called --nouml and apply to both PlantUML and yUML.

The gen-markdown generator has also been deprecated (it works, but it's not the default markdown documentation generator anymore) in favor of gen-doc which uses mermaid (for UML diagramming) and the jinja2 templating language. Apologies the markdown documentation guide hasn't been updated in a while: https://linkml.io/linkml/generators/markdown.html

@Silvanoc
Copy link
Contributor Author

@Silvanoc: thank you for your work on this! I didn't know about the plantuml project before this, but it looks like another tool with UML diagram generation capabilities. Currently, the de facto tool that we're using within the linkml framework to generate/render UML diagrams is mermaid. The only kind of diagrams that we are rendering as part of the web documentation generator (gen-doc/docgen.py) at the moment are class diagrams (granted showing minimal relationship information, simply because we haven't prioritized it enough to improve the level of detail it shows).

I'm aware of it and I'm not trying to provide here a replacement for it, but offering an alternative with some advantages when talking about top-level diagrams for huge schemas.

I have a couple of questions for you -

  1. Does the plantUML generator you wrote show more comprehensive linking and provide more valuable information than the current mermaid diagrams?

They are very similar WRT the diagrams that you can generate and their features. So WRT this question I wouldn't say that PlantUML has any remarkable advantage.

  1. Why not focus on improving the detail in the mermaid diagrams rather than introducing another UML diagramming tool? Is there something lacking in mermaid that plantUML provides?

The main difference is that Mermaid is client generated on visualization (on the browser) and PlantUML is generating a diagram on build-time.

Build-time diagram generation has a couple of advantages:

  • Less client-load
  • Possibility to get an image that can be visualized with a different tool with zoom capabilities. That's relevant if you have huge and complex schemas, where the Mermaid approach IMO doesn't scale. If the individual elements are too small, bad luck.
  • Easier to diagnose why a diagram is not showing as expected. The hereby contributed code let you generate PlantUML that you can review and use to diagnose issues by simply removing parts and feeding the rest to a PlantUML server.

There's also an ER diagram generator in the framework: https://github.com/linkml/linkml/blob/main/linkml/generators/erdiagramgen.py

@Silvanoc
Copy link
Contributor Author

The gen-markdown generator has also been deprecated (it works, but it's not the default markdown documentation generator anymore) in favor of gen-doc which uses mermaid (for UML diagramming) and the jinja2 templating language. Apologies the markdown documentation guide hasn't been updated in a while: https://linkml.io/linkml/generators/markdown.html

Even better, I can remove the changes to that file from the patch 👍

@Silvanoc Silvanoc force-pushed the add-plantuml-support branch from 163b2d0 to 224f697 Compare June 26, 2023 11:14
@dalito
Copy link
Member

dalito commented Jun 26, 2023

@sujaypatil96 - FYI ,with sphinxcontrib.plantuml the plantuml diagrams can be rendered from *-puml files as svg (or other formats) and embedded in the docs. But there are probably no strong arguments for swtiching linkml-docs.

However, for users like myself that prefer plantuml this PR would be a nice addition.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Jun 26, 2023

@sujaypatil96 WRT the deprecation of gen-markdown in favor of gen-doc, I think that the documentation should be updated accordingly. As of now I only find three references to gen-doc in the documentation and they cannot be considered proper documentation:

  1. https://linkml.io/linkml/generators/markdown.html#alternatives
  2. https://linkml.io/linkml/faq/tools.html#can-i-include-generated-documentation-in-a-sphinx-site
  3. https://linkml.io/linkml/faq/tools.html#can-i-customize-the-markdown-generation-for-my-schema-site

Additionally, I'm miserably failing to the the top-level UML with --include-top-level-diagram --diagram-type uml_class_diagram. I'm getting the error NotImplementedError: This is currently handled in the jinja templates, and I don't know how to tackle it. But that might me only me not capable of getting what the tool is telling me.

Even with ER-Diagram I'm failing and I don't really know why. If I copy the result from the Markdown to an online editor, it's being properly rendered. But not on my generated pages.

@cmungall
Copy link
Member

@sujaypatil96 can you give an update on @Silvanoc's comments above?

I think the option to have plantuml as an optional output is most welcome

@sujaypatil96
Copy link
Member

@Silvanoc: yup, sorry the documentation's been so far behind. We've updated the linkml docs to capture the team's current recommendations on markdown documentation generation here: https://linkml.io/linkml/generators/markdown.html

That being said, based on @dalito and @cmungall's suggestions it would definitely be a nice option to have the markdown documentation generator render either Mermaid or plantuml diagrams in the generated web docs. The way you've developed it currently (as a separate module plantumlgen.py) is perfect. We can call this generator within docgen.

It would be nice if we could have unit tests for this generator. There's some documentation in our contributor guidelines: https://linkml.io/linkml/contributing/contributing.html#linkml-testing-framework, but I'm happy to work on this with you.

@Silvanoc
Copy link
Contributor Author

It would be nice if we could have unit tests for this generator. There's some documentation in our contributor guidelines: https://linkml.io/linkml/contributing/contributing.html#linkml-testing-framework, but I'm happy to work on this with you.

I plan to add unit tests. But as of now I have higher prio tasks in an ongoing project.

@Silvanoc Silvanoc marked this pull request as draft July 19, 2023 21:22
@Silvanoc
Copy link
Contributor Author

Marked as draft because adding unittests and fixing issues detected adding the tests.

@Silvanoc Silvanoc force-pushed the add-plantuml-support branch 4 times, most recently from 1727192 to cb4067c Compare July 20, 2023 09:11
@Silvanoc Silvanoc marked this pull request as ready for review July 20, 2023 09:19
@Silvanoc
Copy link
Contributor Author

It would be nice if we could have unit tests for this generator. There's some documentation in our contributor guidelines: https://linkml.io/linkml/contributing/contributing.html#linkml-testing-framework, but I'm happy to work on this with you.

@sujaypatil96 now the unit tests are also in place. The GitHub pipelines are failing only due to the flaky codecov.

@sujaypatil96
Copy link
Member

@Silvanoc: apologies for the late review on this PR, I/the team got caught up with other priorities and this sort of slipped through the cracks. We're bringing it back onto the front burner, and want to get this merged in at the earliest. Thank you for adding tests for your generator.

I cloned your fork of the repo, switched over to the branch where you have the plantUML generator and generated a UML visualization for the kitchen_sink.yaml schema, which is the test schema that most of our generators use. It did a fantastic job of generating a UML diagram. I checked the diagrams generated by both your generator, as well as the existing yUML generator for feature parity and I saw that everything checked out nicely.

The reason why GitHub Actions is complaining right now on this PR is because you have some upstream changes that you need to bring into your fork: Silvanoc#1, and doing so will get rid of the ruff linting error that Actions is complaining about.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Add some basic unit tests for the PlanUML code generator.

As of now SVG diagram generation is not being tested.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
@Silvanoc Silvanoc force-pushed the add-plantuml-support branch from cb4067c to 9fab2b9 Compare August 22, 2023 06:04
@Silvanoc
Copy link
Contributor Author

Silvanoc commented Aug 22, 2023

@Silvanoc: apologies for the late review on this PR, I/the team got caught up with other priorities and this sort of slipped through the cracks. We're bringing it back onto the front burner, and want to get this merged in at the earliest. Thank you for adding tests for your generator.

No problem, I appreciate very much your effort to process all PRs. It's normal that those that are not in any critical path are left behind for a while.

The reason why GitHub Actions is complaining right now on this PR is because you have some upstream changes that you need to bring into your fork: Silvanoc#1, and doing so will get rid of the ruff linting error that Actions is complaining about.

@sujaypatil96 I've noticed that your PR was equivalent to rebasing onto you upstream main. Therefore I've simply rebased instead of accepting your PR in order to have a cleaner git history. I don't like PRs to a PR-Branch simply to sync with the target branch. I very much prefer rebasing. I hope it's fine for you.

@sujaypatil96
Copy link
Member

@sujaypatil96 I've noticed that your PR was equivalent to rebasing onto you upstream main. Therefore I've simply rebased instead of accepting your PR in order to have a cleaner git history. I don't like PRs to a PR-Branch simply to sync with the target branch. I very much prefer rebasing. I hope it's fine for you.

Great! rebasing works just as well, in fact it's better because it preserves linear git history. Merging your PR in 🚀

@sujaypatil96 sujaypatil96 self-requested a review August 24, 2023 17:03
@sujaypatil96 sujaypatil96 merged commit eb718ef into linkml:main Aug 24, 2023
@Silvanoc Silvanoc deleted the add-plantuml-support branch August 25, 2023 05:33
@VladimirAlexiev
Copy link
Contributor

VladimirAlexiev commented Apr 9, 2024

hi @Silvanoc and @sujaypatil96 !
I know a lot about PlantUML, so I can help to improve the look.
Eg https://linkml.io/linkml/generators/plantumlgen.html can be improved if you add this preamble and remove quotes around field names and types (like here):

hide circle
hide empty members

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Apr 9, 2024

hi @Silvanoc and @sujaypatil96 ! I know a lot about PlantUML, so I can help to improve the look. Eg https://linkml.io/linkml/generators/plantumlgen.html can be improved if you add this preamble (like here):

hide circle
hide empty members

@VladimirAlexiev I like your proposal, please send a PR with the proposal

@VladimirAlexiev
Copy link
Contributor

@Silvanoc Why do you use Kroki and not https://www.plantuml.com/plantuml/ ? Unlike kroki, that one always has the very latest version of plantuml.

kroki_server: Optional[str] = "https://kroki.io"

@VladimirAlexiev
Copy link
Contributor

send a PR with the proposal

#2060

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Apr 9, 2024

@Silvanoc Why do you use Kroki and not https://www.plantuml.com/plantuml/ ? Unlike kroki, that one always has the very latest version of plantuml.

kroki_server: Optional[str] = "https://kroki.io"

@VladimirAlexiev I use Kroki in order to support self-hosted diagram generators. That is in fact what we use in our company.
If I'm right, the APIs are slightly different and I didn't want to add support for both.

@dalito
Copy link
Member

dalito commented Apr 9, 2024

@VladimirAlexiev see also #1956 for plantuml-related problems/plans.

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

Successfully merging this pull request may close these issues.

6 participants