-
Notifications
You must be signed in to change notification settings - Fork 129
General: custom staging dir functionality migration #5207
General: custom staging dir functionality migration #5207
Conversation
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.
Not sure about the changes and whether it improves any readability, especially the circular imports (and it being worked around) feels problematic.
What stood out to me is that get_instance_staging_dir
does not rely on the custom staging dir info at all nor the other way around. Which makes me wonder how do I actually get the correct staging directory for an instance. I'd have expected that get_instance_staging_dir
would've considered also the custom transient dir setting, etc. but it seems that's still up to the implementation to check first whether an instance should have a custom staging dir, and if not.. then use get_instance_staging_dir
which seems confusing from the name of that function.
openpype/pipeline/publish/lib.py
Outdated
# deprecated: backward compatibility only | ||
# TODO: remove in the future | ||
def get_instance_staging_dir(instance): | ||
return _get_instance_staging_dir(instance) | ||
|
||
|
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.
# deprecated: backward compatibility only | |
# TODO: remove in the future | |
def get_instance_staging_dir(instance): | |
return _get_instance_staging_dir(instance) |
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.
Are you suggesting we don't need to maintain compatibility for this function?
**kwargs: Arbitrary keyword arguments. See below. | ||
|
||
Keyword Arguments: | ||
force_temp (bool): If True, staging dir will be created as tempdir. |
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.
How come we are not adding them as optional arguments?
) | ||
else: | ||
ctx_data = get_template_data_with_names( | ||
project_name, asset_name, task_name, system_settings |
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.
Should we be passing the host_name
here as well?
# get customized tempdir path from `OPENPYPE_TMPDIR` env var | ||
custom_temp_dir = _create_custom_tempdir( | ||
anatomy.project_name, anatomy) | ||
|
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 think this could be only for both? And then _create_local_staging_dir
is no longer needed :
return os.path.normpath(
tempfile.mkdtemp(
prefix=prefix,
suffix=suffix,
dir=custom_temp_dir
)
)
If _create_custom_tempdir
returns None, so we don't need to do the conditional check.
openpype/pipeline/create/utils.py
Outdated
""" | ||
|
||
output = { | ||
instance.id: -1 if use_value_for_missing else None |
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.
Here we are assuming all of them are missing, right? But later on I can't see where do we check for missing entities? So it could be that we return an instance with -1
that has entities? i.e. Line 50
openpype/pipeline/__init__.py
Outdated
@@ -194,5 +208,5 @@ | |||
|
|||
# Backwards compatible function names | |||
"install", | |||
"uninstall", | |||
"uninstall" |
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.
We add the comma, so a future PRs which add after or move it does not have to add commit to the line.
create_ctx = self.create_context | ||
asset_name = instance.get("asset") | ||
subset = instance.get("subset") | ||
if not any([asset_name, subset]): |
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 actually faster and easier to read.
if not any([asset_name, subset]): | |
if not asset_name or not subset: |
openpype/pipeline/create/utils.py
Outdated
if version == -2: | ||
output[instance_id] = None | ||
elif version == -1: | ||
output[instance_id] = 1 |
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 has changed recently and requires get_versioning_start
from openpype.pipeline.version_start
.
no reason to combine camel and snake case together
6967ca4
to
14f89e7
Compare
@@ -723,6 +725,58 @@ def get_pre_create_attr_defs(self): | |||
return self.pre_create_attr_defs | |||
|
|||
|
|||
def apply_staging_dir(self, instance): |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
template_name: str = Field( | ||
"", | ||
title="Template Name", | ||
placeholder="Shared templates: ayon+anatomy://templates/staging_directories" |
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.
line too long (84 > 79 characters)
Changelog Description
The functionality of the staging directory has exceeded its original purpose of publishing. Considering its increased usage in the creation process, we need to migrate it to a dedicated module within the pipeline parent.
Additional info
Testing notes: