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

Global: supporting OPENPYPE_TMPDIR in staging dir maker #4398

Conversation

jakubjezek001
Copy link
Member

@jakubjezek001 jakubjezek001 commented Jan 30, 2023

Brief description

Productions can use OPENPYPE_TMPDIR for staging temp publishing directory

Description

Studios were demanding to be able to configure their own shared storages as temporary staging directories.
Template formatting is also supported with optional keys formatting and following anatomy keys:
- root[work | ]
- project[name | code]

Documentation:

future documentataion url

Testing notes:

  1. configure OPENPYPE_TMPDIR on a testing project with {root[work]}/{project[name]}/temp
  2. publish anything from any host.
  3. all should work as expected.
  4. notice temp foder in root of your project will appear.

@ynbot
Copy link
Contributor

ynbot commented Jan 30, 2023

@jakubjezek001 jakubjezek001 self-assigned this Jan 30, 2023
@jakubjezek001 jakubjezek001 marked this pull request as draft January 30, 2023 12:50
@jakubjezek001 jakubjezek001 marked this pull request as ready for review January 30, 2023 12:56
jakubjezek001 and others added 2 commits January 30, 2023 13:56
Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Nice feature! :)

Is it to be expected that there are no Studio Settings to set the temp dir and its up to the machine itself to "choose" the temp directory by setting a global OPENPYPE_TEMP_DIR variable from outside of OpenPype itself?

I like that a particular user/machine can set its own custom temporary directory, e.g. for if that machine has a strong local cache disk to use. But I felt that with the complex formatting that is possible using anatomy this feels more like a studio setting of sorts with a potential override in local settings?

Anyway, only other thing I'm worried about is that it breaks some logic I had set up for Substance Painter. In particular this line of code of using publish.get_instance_staging_dir(instance) from within a Collector because that is in a CollectorOrder BEFORE instance.context.data["anatomy"] already exists. Keeping that in mind this might be a change breaking other areas of code too.

EDIT:

Missing documentation!

Also, this feature must have a mention in the documentation too. So please make sure the Admin Docs contain an explicit mention about the feature and how it's intended to be used by a studio or artist.

openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
@jakubjezek001
Copy link
Member Author

Is it to be expected that there are no Studio Settings to set the temp dir and its up to the machine itself to "choose" the temp directory by setting a global OPENPYPE_TEMP_DIR variable from outside of OpenPype itself?

Yes that was the idea. Yes the documentation would be important for this.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 8, 2023

Few comments:

  • Current imlementation is using OPENPYPE_TMPDIR only at one place and there is no prepared function to get it from any other place. The implementation expects that it is used only for instance staging dir but it should be possible to get it out of instance scope. So it is not usable for other places where new temp directory should be created without using instance staging dir.

  • The value is targeted for publishing and nothing else in that case it should be maybe called OPENPYPE_PUBLISH_TMPDIR because I would expect more usages then just publishing.

  • The possible keys is overkill which is limiting usage of the value. I would limit possible keys only to project name/code and roots. I can't imagine any usecase where other keys from anatomy data would make sense.

  • I would expect a function to just create tempdir (e.g. in openpype/pipeline/tempdir.py -> just made up) which is then used in publish staging dir.

def _format_tempdir(temp_dir, project_name, anatomy):
    if "{" not in temp_dir:
        return temp_dir

    if anatomy is None:
        anatomy = Anatomy(project_name)

    data = {
        "root": anatomy.roots,
        "project": {
            "name": project_name,
            "code": anatomy.project_code
        }
    }
    return StringTemplate.format_template(temp_dir, data).normalized()


def create_tempdir(project_name, anatomy=None, prefix=None):
    if prefix is None:
        prefix = "op_tmp_"

    kwargs = {"prefix": prefix}
    openpype_temp_dir = os.getenv("OPENPYPE_TMPDIR")
    if openpype_temp_dir:
        openpype_temp_dir = _format_tempdir(
            openpype_temp_dir,
            project_name,
            anatomy
        )
        if not os.path.exists(openpype_temp_dir):
            os.makedirs(openpype_temp_dir)
        kwargs["dir"] = openpype_temp_dir

    return os.path.normpath(
        tempfile.mkdtemp(**kwargs)
    )

jakubjezek001 and others added 2 commits February 10, 2023 13:42
Co-authored-by: Jakub Trllo <43494761+iLLiCiTiT@users.noreply.github.com>
@iLLiCiTiT iLLiCiTiT assigned antirotor and unassigned jakubjezek001 Feb 10, 2023
Copy link
Member

@antirotor antirotor left a comment

Choose a reason for hiding this comment

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

Some small typos.

Question - shouldn't this be set per context? I mean - it would be useful to influence the staging dir per family (or asset). Considering cases like special share for transient renders or big caches...

openpype/pipeline/tempdir.py Outdated Show resolved Hide resolved
openpype/pipeline/tempdir.py Outdated Show resolved Hide resolved
openpype/pipeline/tempdir.py Outdated Show resolved Hide resolved
openpype/pipeline/tempdir.py Outdated Show resolved Hide resolved
openpype/pipeline/tempdir.py Outdated Show resolved Hide resolved
openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
openpype/pipeline/publish/lib.py Outdated Show resolved Hide resolved
website/docs/admin_environment.md Outdated Show resolved Hide resolved
website/docs/admin_settings_system.md Outdated Show resolved Hide resolved
@iLLiCiTiT
Copy link
Member

Question - shouldn't this be set per context? I mean - it would be useful to influence the staging dir per family (or asset). Considering cases like special share for transient renders or big caches...

It is still creating subdirectories in the temp dir, and must create them because of filename clashes.

@antirotor antirotor assigned jakubjezek001 and unassigned antirotor Feb 10, 2023
jakubjezek001 and others added 10 commits February 13, 2023 12:42
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
@jakubjezek001 jakubjezek001 merged commit 827e868 into develop Feb 14, 2023
@jakubjezek001 jakubjezek001 deleted the feature/OP-3663_Hiero-export-of-large-number-of-shots-fill-up-disk branch February 14, 2023 14:35
@ynbot ynbot added this to the next-patch milestone Feb 14, 2023
@antirotor
Copy link
Member

This is unfortunately breaking Python 2 hosts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Hiero type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants