-
Notifications
You must be signed in to change notification settings - Fork 129
Shotgrid: Add production beta of shotgrid integration #2921
Conversation
cf47c91
to
f258d1a
Compare
openpype/modules/deadline/plugins/publish/submit_publish_job.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested the module but did a quick glance over the code changes. Commented some things that stood out and also had these notes:
I feel like the addition of the following files don't belong to this PR:
This file seems to be doing nothing of use:
The file openpype/settings/entities/enum_entity.py
has a lot of code style changes unrelated to the "logic change". I'm fine with the changes since a lot of them help readability. Just wanted to note that it tweaks quite a few lines of code that aren't absolutely required for this PR. Likely it's an automated thing from your code editor.
openpype/modules/shotgrid/plugins/publish/collect_shotgrid_session.py
Outdated
Show resolved
Hide resolved
def get_shotgrid_servers() -> Dict[str, Any]: | ||
return get_shotgrid_settings().get("shotgrid_settings", {}) | ||
|
||
|
||
def get_leecher_backend_url() -> str: | ||
return get_shotgrid_settings().get("leecher_backend_url") | ||
|
||
|
||
def filter_projects_by_login() -> bool: | ||
return bool(get_shotgrid_settings().get("filter_projects_by_login", False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions won't be very optimized - it might make sense more to instead add the result of get_shotgrid_settings
as an argument. That way you don't need constantly requery the database for the settings (which can be quite a lot of data to query).
It's not too many calls currently that'd get reduced - but it'd remove a call here since the full settings just got queried a few lines above.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions won't be very optimized
Premature optimization is the root of all evil (or at least most of it) in programming - Donald Knuth
We are talking about usage of MongoDB, which is reputed of being highly performant and has strongly optimized throughput.
Despite the fact that it's a premature optimization, I've added LRU cache for those functions.
f258d1a
to
1156b6f
Compare
openpype/modules/deadline/plugins/publish/submit_publish_job.py
Outdated
Show resolved
Hide resolved
I would like to avoid those stylish changes, but the hound didn't leave us any choice with its "lines too long" checks. So, black formatted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code updates look good to me - unfortunately I can't test it in production myself.
I did have a question about the Py3 type hints in the code base. I like it, however I'm not sure we can adopt that already. The collectors would run within the hosts - e.g. Maya 2020, with Py 2.7 - and would import code of the module and thus fails with SyntaxErrors. Or would that actually work currently? It might be nice to test the logic/functionality in a Py2 host to be sure. The same for f-string formatting.
It would be a bit of a code style loss if we'd need to preserve Py2 in that area - but we might still need to currently? Thoughts?
Let me first accomplish few tests under Maya 2018 and come back here with more elements. |
7a14643
to
d106ba4
Compare
@pberto were you able to give this a go? |
This is something we will test soon. |
@victorbartel thank you for the work on this. We're unable to test this internally at the moment, however as soon as we get a second opinion from @pberto or someone else, we're happy to move forward and merge this in, considering how well self contained it is! |
We plan to test it the second week of MAY as we need to be on cutonmpr site. |
_LOG = Logger().get_logger("ShotgridModule.server") | ||
|
||
|
||
def find_linked_projects(email): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function
return {} | ||
|
||
|
||
def _get_shotgrid_task(avalon_project, avalon_asset, avalon_task): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have few questions regarding this function.
- Why is shotgrid creating separated mongo database for data that should be in avalon database?
At least it looks it's done that way. I maybe missed that part of shotgrid implementation? - Why
_id
contains strings?
Few comments:
Please try avoid using avalon
in variables and also be more specific about the content (e.g. asset_doc
or task_name
tells more about it's content), also pass project document just to use it's name can be simplified to pass just the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my memory, it seemed simpler ans more maintainable for @victorbartel to store shotgrid data in another database and store in avalon on the asset that corresponds to the id of this database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense in SQL database, not in mongo.
Now you have to query twice from 2 different databases to get data related to the same entity. To get shotgun entity based on asset name you have to query it from avalon database to get it's id, then based on the id query it's shotgrid id from shotgrid database and then query it from shotgrid server? Performance of that with cloud mongo wouldn't be great.
And _id
key is not ObjectId
but string, which is not requirement, but the ObjectId is used for indexing in mongo collections.
openpype/modules/shotgrid/plugins/publish/collect_shotgrid_entities.py
Outdated
Show resolved
Hide resolved
0163eae
to
47d2a61
Compare
Update and revert poetry lock changes
Added shotgun-api3 source files to poetry lock
@@ -131,6 +131,13 @@ | |||
} | |||
} | |||
}, | |||
"shotgrid": { | |||
"enabled": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module should be disabled by default.
"enabled": true, | |
"enabled": false, |
@@ -131,6 +131,13 @@ | |||
} | |||
} | |||
}, | |||
"shotgrid": { | |||
"enabled": true, | |||
"filter_projects_by_login": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused setting right now
@ClementHector I believe the last are the two small things regarding settings that @iLLiCiTiT commented about |
I made the changes for the settings, for the base in the mongo I will try to do this in the next two weeks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few thing that require some attention, but that's for future PRs. I would say this is ready to merge.
Brilliant. Thank yo guys for this. It took a long while, but is a brilliant addition to OpenPype. I'm happy to merge this. |
Thanks for the major contribution. Would love to see some demo videos of this in action! :) |
Brief description
Add shotgrid module
Description
The purpose of this PR is to add a brand-new module which allows synchronizing data from Shotgrid to Openpype.
Additional info
This module is still under development and is not in its final shape. Main features to be covered are:
Incremental event synchronisation (Shotgrid to Avalon)Documentation (add "type: documentation" label)
module documentation
server documentation
Testing notes:
tools/run_tray.sh