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

Allow multiple services #18

Merged

Conversation

EmberLightVFX
Copy link
Contributor

This PR prepares the service to be able to run multiple different services.
It's needed for the next PR I'm working on to implement Gazu event listeners so Ayon will get the same kitsu sync as OpenPype.

Most updates in the code is inspired by the shotgrid code (https://github.com/ynput/ayon-shotgrid)

@EmberLightVFX EmberLightVFX mentioned this pull request Feb 5, 2024
@martastain martastain requested a review from iLLiCiTiT February 6, 2024 08:55
@martastain
Copy link
Member

I see no problem with this PR itself (thanks for the clean-up), but I'm not convinced that the sync trigger justifies another service. The overhead of having two minimalistic services (not only computational but work required to set them up, keeping two separate images...) is pretty high - don't you think, we could just run the listener in another thread of the processor service? @iLLiCiTiT wdyt?

@martastain martastain self-requested a review February 6, 2024 09:00
@EmberLightVFX
Copy link
Contributor Author

Hmm, maybe. I was mostly looking on how the shotgrid addon. It has 3 different services.
I don't see any problem running the sync-service and the current processor running from the same service. Maybe it's just cleaner code to have them separate?
Ether way, this PR can still be nice to have for any future custom service-creation for Kitsu and what ever we decide for the sync-service (split or combined) that's for my next PR :)

services = {
"processor": {"image": f"ynput/ayon-kitsu-processor:{__version__}"}
}
services = {"processor": {"image": f"ynput/ayon-kitsu-processor:{__version__}"}}
Copy link
Member

Choose a reason for hiding this comment

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

I actually think that the previous formatting was better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed ;)
I added a , after the processor dict. That way Black will format it per line.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 7, 2024

don't you think, we could just run the listener in another thread of the processor service?

We have listener as separated service in ftrack so you can restart the processor service without loosing events which happened meanwhile the processor is restarting.

But if listener is not listening to "live" events, but is querying them, then it does not make sense and can be in single service.

@@ -0,0 +1,6 @@
class KitsuServerError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file has to be out of the service itself, there are only exceptions...

Copy link
Contributor Author

@EmberLightVFX EmberLightVFX Feb 7, 2024

Choose a reason for hiding this comment

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

I put it outside of the processor service as they are also used in my sync_service code.
I thought it would be good to place them in the kitsu_common so we don't have them duplicated.

Copy link
Member

@iLLiCiTiT iLLiCiTiT Feb 7, 2024

Choose a reason for hiding this comment

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

I put it outside of the processor service as they are also used in my sync_service code.

I don't understand? The exceptions are used only in processor service. Do you have you're own sync_service service? If yes, how does help to have it in kitsu_common? You can't raise exception in one process and catch it in other...

If you have it in same process then you can probably do import like from processor.exceptions import ... instead of from kitsu_common import ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classes and functions that I will use in my upcoming PR (that's currently called sync_service (Same name as the Kitsu sync service in OpenPype)) I placed in untils.py
Mostly to skip duplicated code between multiple services. Same thing here, inspired on how the Shotgrid addon is made.

Copy link
Member

@iLLiCiTiT iLLiCiTiT Feb 7, 2024

Choose a reason for hiding this comment

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

Hard to tell without the multiple usage. Can you move it to common in the upcomming PR? It does not make sense here and it is not possible to comment if it's ok or not without the usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sure thing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@EmberLightVFX
Copy link
Contributor Author

EmberLightVFX commented Feb 7, 2024

But if listener is not listening to "live" events, but is querying them, then it does not make sense and can be in single service.

The Gazu listeners executes a function when being called. It's possible to ether work "live" or if Ayon have a queue system we can use that.
I rather go with the live approach as it's easier to move the code from OP to Ayon

@martastain martastain merged commit 283cfe1 into ynput:develop Feb 8, 2024
@EmberLightVFX EmberLightVFX deleted the enhancement/allow_multiple_services branch February 8, 2024 09:57
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.

3 participants