-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Graphviz fix #7843
Graphviz fix #7843
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these fixes! I tested Docker build - everything is fine.
Hi @titu1994, I've addressed your comments. |
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs discussing about license (non standard), necessity (why is a graph visualization library a required dependency for K2 ??) And whether it's realistic to bloat the apt get section wth so many libraries.
The licence is |
This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days. |
This PR was closed because it has been inactive for 7 days since being marked as stale. |
Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com>
Hi all! |
It's not been discussed yet - why is a graph visualization library being installed in the container ? |
Simply for convenient prototyping. I always work inside NeMo containers (maybe I'm not the only one in that) and I'd like to have graphviz already installed rather than having to manually install it every time I run a new image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the clarification and legal review. Looks good to merge
* graphviz reinstallation added Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * fix comments Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove redundant installations Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * additional dependencies are put into a separate RUN Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> --------- Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> Co-authored-by: Vladimir Bataev <vbataev@nvidia.com>
* graphviz reinstallation added Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * fix comments Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove redundant installations Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * additional dependencies are put into a separate RUN Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> --------- Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> Co-authored-by: Vladimir Bataev <vbataev@nvidia.com> Signed-off-by: Sasha Meister <ameister@nvidia.com>
* graphviz reinstallation added Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * fix comments Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove redundant installations Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * remove rebase glitch Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> * additional dependencies are put into a separate RUN Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> --------- Signed-off-by: Aleksandr Laptev <alaptev@nvidia.com> Co-authored-by: Vladimir Bataev <vbataev@nvidia.com>
What does this PR do ?
Collection: [ASR]
Changelog
Usage
# Add a code snippet demonstrating how to use this
Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information