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

[cuegui] feat: Add job node graph plugin v2 #1400

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

lithorus
Copy link
Contributor

@lithorus lithorus commented Jun 29, 2024

Link the Issue(s) this Pull Request is related to.
#888 (original PR)

Summarize your change.
This is an adapted PR to support QtPy and PySide6

It depends a fork of NodeGraphQt that has been adapted to use QtPy instead of Qt directly.

@lithorus
Copy link
Contributor Author

It's pulling in a specific fork of NodeGraphQt which is not optimal. The question is wether or not it's better to just import the code from the fork instead of referencing an external repo (without releases).
It's licensed under MIT, so it should be ok?

@lithorus
Copy link
Contributor Author

lithorus commented Jun 29, 2024

Obviously it fails the python 2 tests, but the PySide6 tests fails because of missing git command in the opencue:2019 image (due to the NodeGraphQt python module directly from a git repo).

@lithorus
Copy link
Contributor Author

lithorus commented Jun 30, 2024

After I imported and modified the NodeGraphQt package I got it to pass all tests (including a simple one for the new graph), however the python2 ones doesn't test the GUI, so not sure if it will still work with python2.

Have only tested locally with PySide6 and not sure if it will work with PySide2.

@lithorus lithorus marked this pull request as ready for review June 30, 2024 00:32
@lithorus lithorus closed this Jul 10, 2024
@lithorus lithorus reopened this Jul 10, 2024
@DiegoTavares
Copy link
Collaborator

After I imported and modified the NodeGraphQt package I got it to pass all tests (including a simple one for the new graph), however the python2 ones doesn't test the GUI, so not sure if it will still work with python2.

Have only tested locally with PySide6 and not sure if it will work with PySide2.

I wouldn't worry about python2 now, we're un"officially" not supporting it anymore.

@DiegoTavares
Copy link
Collaborator

DiegoTavares commented Sep 11, 2024

It's pulling in a specific fork of NodeGraphQt which is not optimal. The question is wether or not it's better to just import the code from the fork instead of referencing an external repo (without releases). It's licensed under MIT, so it should be ok?

Commiting the fork as part of this project is not the right approach IMO, as it would prevent us from keeping thing in sync. I see two options to make this dependency maintainable.

  1. Bring the NodeGraphQt fork in as a git submodule
  2. Make downloading the fork part of the CI/CD pipeline

Although git submodules are a pain to maintain, I still think 1 is the best option, at least while there's no official release available.

Copy link
Collaborator

@DiegoTavares DiegoTavares left a comment

Choose a reason for hiding this comment

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

Bring NodeGraphQt fork as a submodule.

@lithorus lithorus marked this pull request as draft September 11, 2024 19:25
@lithorus
Copy link
Contributor Author

Looks like there has also been some updates to the pyside6 fork, will test with the latest changes to see if it might work

@lithorus
Copy link
Contributor Author

A did a quick test and atleast with PySide6 it seems to work with just the updated fork. Will need to test it with PySide2 to see if it still requires changes.

@lithorus
Copy link
Contributor Author

I had to remove the pyside6 test and upgrade python linting to CY2023 (from CY2022) to get all the tests to complete.
The changes also need changes to the documentation on how to clone with submodules

@lithorus lithorus marked this pull request as ready for review September 11, 2024 22:17
@lithorus lithorus changed the title feat: Add job node graph plugin v2 [cuegui] feat: Add job node graph plugin v2 Sep 19, 2024
@DiegoTavares
Copy link
Collaborator

I'm planning to test this feature this week in the SPI environment to confirm we're able to bring the submodule and build it locally. If this works without any issues, I'm okay in merging this in the way it is. If I find it will be a pain to carry this dependency to our environment where downloading things from github is blocked at the network level, we might need to add changes to make this dependency optional.

@lithorus lithorus marked this pull request as draft October 23, 2024 19:03
@lithorus
Copy link
Contributor Author

lithorus commented Dec 3, 2024

I have published a fork of NodeGraphQt as NodeGraphQytPy on pypi, which is using QtPy instead of Qt and have been using that instead of NodeGraphQt

@lithorus lithorus marked this pull request as ready for review December 3, 2024 20:25
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.

2 participants