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

[rqd] [cuegui] Consolidate log read and write into a single package #1474

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

Conversation

lithorus
Copy link
Contributor

@lithorus lithorus commented Aug 14, 2024

Summarize your change.
This moves log reading and writing into a single package, to make it easier to support multiple backends in a single place.

LogViewPlugin.LogReader and rqdlogging.RQDLogger have been replaced with cuelogging.CueLogReader and cuelogging.CueLogWriter

rqd.compiled_proto has also been replaced with opencue.compiled_proto in rqd

Have also updated tests and documentation.

@lithorus lithorus changed the title Consolidate loggers into a single class Consolidate log read and write into a single class Aug 14, 2024
@lithorus lithorus marked this pull request as draft August 14, 2024 20:47
@lithorus lithorus marked this pull request as ready for review August 14, 2024 22:55
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.

Now that I'm looking at your proposal materialized, I have some concerns about how it will impact the project in the future.

  • Up until now, rqd was independent of pycue, which means the only contract they had in common was the protobuf layer (Loose coupling). With this change, they now share modules, which is good for reusability, but increases the coupling. If we had a great portion of code being reused, maybe the tradeoff would be fair, but so far we're only reusing cuelogging on READ mode. Which brings me to my next concern
  • On cuelogging we're exposing the WRITE mode to pycue, which doesn't have any use for it. This would be harmless if we didn't have artists that like to program and hack things, I can easily see an user of the API accidentally initializing CueLogger in WRITE mode and inadvertently triggering a log rotate (which is part of the __init__ function).

I'm really sorry I didn't catch this concerns on the design phase, but I do think keeping pycue(opencue) and rqd as separate modules that only communicate via protobuf weights heavier on the tradeoff between reuse and coupling.

pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
pycue/opencue/cuelogging.py Outdated Show resolved Hide resolved
@lithorus lithorus marked this pull request as draft August 20, 2024 19:04
@lithorus
Copy link
Contributor Author

lithorus commented Aug 20, 2024

Now that I'm looking at your proposal materialized, I have some concerns about how it will impact the project in the future.

* Up until now, rqd was independent of pycue, which means the only contract they had in common was the protobuf layer (Loose coupling). With this change, they now share modules, which is good for reusability, but increases the coupling. If we had a great portion of code being reused, maybe the tradeoff would be fair, but so far we're only reusing cuelogging on READ mode. Which brings me to my next concern

* On cuelogging we're exposing the WRITE mode to pycue, which doesn't have any use for it. This would be harmless if we didn't have artists that like to program and hack things, I can easily see an user of the API accidentally initializing `CueLogger` in WRITE mode and inadvertently triggering a log rotate (which is part of the `__init__` function).

I'm really sorry I didn't catch this concerns on the design phase, but I do think keeping pycue(opencue) and rqd as separate modules that only communicate via protobuf weights heavier on the tradeoff between reuse and coupling.

No worries. I think I've re-written this a few times already :)

  • Having opencue as a dependency, was partly the reason I brought up the dependencies allowed in Changes to requirements.txt and define dependencies #1441. The plan was that the cuelogging.py could be overwritten (by the studio) with a custom one to provide both read and write. I'll create a proof-of-concept of this later. (Have added example on how to override it in the description)

  • I understand the concern (and wasn't 100% happy with it either), but the main point was to have read and write in the same file atleast. Would it be a better idea to have a CueLogReader and a CueLogWriter instead?

@lithorus lithorus marked this pull request as ready for review August 22, 2024 18:17
@lithorus lithorus marked this pull request as draft September 11, 2024 23:33
@lithorus lithorus changed the title Consolidate log read and write into a single class Consolidate log read and write into a single package Sep 12, 2024
@lithorus
Copy link
Contributor Author

Had to move the cuelogging outside of the opencue package so the protobuf's doesn't conflict with each other, but have re-added the compiled_proto to rqd

@lithorus lithorus marked this pull request as ready for review September 12, 2024 00:04
@lithorus lithorus changed the title Consolidate log read and write into a single package [rqd] [cuegui] Consolidate log read and write into a single package Sep 19, 2024
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.

Please also update python_coverage_report.sh and confirm the run_integration_test.sh still works.

One way to test this is to temporarily add the analize_cuebot frompackaging-pipeline.yml to the testing-pipeline.yml. This will run the integration test on the PR.

@lithorus
Copy link
Contributor Author

lithorus commented Oct 9, 2024

analize_cuebot

I added the analyze_python (I guess that's the one you meant?)
Edit:
Looks like it needs a bit of modifiations...

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