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

Resolve Issue with multiple QT backends #166

Closed
wants to merge 3 commits into from

Conversation

ausstein
Copy link
Contributor

@ausstein ausstein commented Oct 9, 2023

As written in the Issues, when installing Matplotlib, I get some qt related errors.

I think the issue is that qtpy does not choose pyside2 by default so when pyqt5 gets installed because of matplotlib, pyqt5 is chosen unless the environment variable is different.

Right now in the code

from ryven.gui_env import init_node_guis_env

from Ryven.py

and

from ryvencore_qt.src.Design import Design

from config.py get called before the environment variable is set.

Right now the choice for the QT_API is in the config.py. And the first call is also in config.py. This leaves two options to fix this:

either set the QT_API environment variable before loading the config. (In that case the qt_api value of the config would be useless)

or set the environment variable in config.py (also not ideal to do this in a config class)

I went with the latter approach for this pull request and made sure the config is loaded before init_node_guis_env is imported

Otherwise importing Design from ryvencore_qt.src.Design can cause a qt error when multiple Qt Backends are available.

It is not Ideal to set an environment variable in the config class but it is a fix until Design is removed.
Load config before importing gui env. Since the environmnet variable is now set in config.py, this ensures the QT_API is set before impoting init_node_guis_env
@leon-thomm
Copy link
Owner

I think config.py imports ryvencore_qt.src.Design because the argument parser calls Config.get_available_flow_themes(). Clearly we cannot make Qt imports when we haven't even parsed arguments yet. For now I think we should just remove this invocation in args_parser.py and hard-code the themes instead.

I agree with moving down the import of the gui_env in Ryven.py, but I'm not quite sure what you intend to do with the change in config.py. As I understand it, we should remove the Qt dependency in the args parser noted above, move down the imports in run() as you did, and set QT_API from config just before that.

@@ -37,6 +38,7 @@ class Config:
window_geometry: Optional[str] = None
window_title: str = 'Ryven'
qt_api: str = 'pyside2'
os.environ['QT_API'] = self.qt_api
Copy link
Owner

Choose a reason for hiding this comment

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

What is your intention here? This code doesn't parse, there is no "self" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, yes the self should not be there. I initially did it differently then changed it. I changed the lines in the conda package and then tried to copy them to the fork manually, I guess that's what you get for not doing it the proper way haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should just be os.environ['QT_API'] = qt_api
but hardcoding the flow themes removes the necessity to set the environment variable in the config so it is no longer needed

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need to hard code anything, see what I did on pr166_tmp

leon-thomm added a commit that referenced this pull request Oct 9, 2023
@leon-thomm
Copy link
Owner

Could you try if branch pr166_tmp works for you? If that fixes it I'll add those commits here and merge the PR.

removed self
@ausstein
Copy link
Contributor Author

yes, I can confirm that the branch works! thank you!

@leon-thomm
Copy link
Owner

Alright, I will close this and merge pr166_tmp instead.

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