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

Fix generic workflow queue entry execute method #870

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

fhernandezvivanco
Copy link

Resolves #869

Copy link
Collaborator

@rhfogh rhfogh left a comment

Choose a reason for hiding this comment

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

Looking at the code, the original intent seems to be that the test is done on workflow_running, and that the change with changes of state are dealt with by the workflow_state_handler. If that is not working properly (as apparently it is not) it would be preferable to either fix the workflow-state_handler (instead of just bypassing it), or removing it entirely and working off the states directly. This change is rather a short-term fix, leaving the code complicated to the next person. Could we check with @marcus-oscarsson , who apparently wrote this to start with?

@marcus-oscarsson
Copy link
Member

marcus-oscarsson commented Mar 15, 2024

This was done before all hardware objects had a standardized state. The proposed change should in theory work for handling the state RUNNING/BUSY however the workflow_state_handler also handles a state called OPEN which means that the workflow requests a dialog to be displayed. I don't think the later is used today (it was used in the Qt interface with EDNA based Workflows).

I think we can try this change but we need to test it properly, and in that case remove the workflow_state_handler method. However if your workflow emits stateChanged then the self.running attribute should be correct, does workflow_state_handler get called at all ?

@marcus-oscarsson
Copy link
Member

What Id suggest is, if you feel comfortable with it (it would make the clean up work for me easier) remove the workflow_state_handler and the connect in pre_execute. Like this we have removed this obsolete piece of code and Ill make a note to test this to make sure that the running state is correct in the end.

@rhfogh
Copy link
Collaborator

rhfogh commented Mar 15, 2024

@marcus-oscarsson Your last proposal sounds fine. Anyway, the GPhL workflow does not use this class, so whatever works, ...

@marcus-oscarsson
Copy link
Member

@fhernandezvivanco: What did you decide to do, are you going for the cleanup ?

@fhernandezvivanco
Copy link
Author

@marcus-oscarsson , sorry for the delay. Happy to clean the class. For some reason I misunderstood and thought you were going for the code cleanup.

@marcus-oscarsson
Copy link
Member

@fhernandezvivanco sorry to bother you with this again, do you think you can rebase this on develop ? and then Ill merge

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.

GenericWorkflowQueueEntry execute method loops infinitely
3 participants