Invalid format specifier crash

[Strangenoise]

Hi BigRoy, thanks for all your work on acre and OpenPype.
I just faced a small and peculiar issue and whished to inform you about it.

Context
I'm deploying rez in my studio and we use OpenPype.
I modified OpenPype to be a rez package and also be able to launch rez packages (instead of applications).
So I've got an acre rez package.

On windows 10, everything is ok.
On linux (Pop!_OS 22.04 LTS), acre crashes when trying to format REZ_STORED_PROMPT_SH environment variable.

With this env var containing: 'REZ_STORED_PROMPT_SH': '\\[\\e]0;\\u@\\h: \\w\\a\\]${debian_chroot:+($debian_chroot)}\\[\\033[01;32m\\]\\u@\\h\\[\\033[00m\\]:\\[\\033[01;34m\\]\\w\\[\\033[00m\\]\\$ .
Note that's an env var generated by rez, not one that I control directly.

Traceback

Traceback (most recent call last):
  File "/home/admin/Documents/rez/packages/build/openpype/4.0.0/python/start.py", line 1208, in <module>
    boot()
  File "/home/admin/Documents/rez/packages/build/openpype/4.0.0/python/start.py", line 1106, in boot
    set_openpype_global_environments()
  File "/home/admin/Documents/rez/packages/build/openpype/4.0.0/python/start.py", line 309, in set_openpype_global_environments
    env = acre.compute(merged_env, cleanup=False)
  File "/home/admin/Documents/rez/packages/build/acre/0.0.1/python/acre/core.py", line 71, in compute
    env[key] = lib.partial_format(env[key], data=data)
  File "/home/admin/Documents/rez/packages/build/acre/0.0.1/python/acre/lib.py", line 45, in partial_format
    return formatter.vformat(s, (), mapping)
  File "/home/admin/Documents/rez/packages/build/python/3.10.12/platform-linux/arch-x86_64/os-Pop-22.04/python/lib/string.py", line 165, in vformat
    result, _ = self._vformat(format_string, args, kwargs, used_args, 2)
  File "/home/admin/Documents/rez/packages/build/python/3.10.12/platform-linux/arch-x86_64/os-Pop-22.04/python/lib/string.py", line 218, in _vformat
    result.append(self.format_field(obj, format_spec))
  File "/home/admin/Documents/rez/packages/build/python/3.10.12/platform-linux/arch-x86_64/os-Pop-22.04/python/lib/string.py", line 235, in format_field
    return format(value, format_spec)
ValueError: Invalid format specifier

Solution proposal
Keep data unformated when they can't be.
In core.py (67:74):

    for key in reversed(result.sorted):
        if key in env:
            data = env.copy()
            data.pop(key)    # format without itself
            try:
                env[key] = lib.partial_format(env[key], data=data)
            except ValueError:
                env[key] = data[key]

[BigRoy]

Thanks for the report.

I'd still like to be able to not pass that completely silently, e.g. logging a warning but I can see that likely cluttering up your logs if this by default is in your envs.

Maybe we should have a ACRE_VERBOSE=1 env var to maybe enable logs like these and then fail more silently by default.

@Strangenoise thoughts?

@antirotor any thoughts on this from AYON/OpenPype's perspective?

Also, the issue is similar to this one #7 (comment) with the request to allow invalid formatting strings {} to pass along without issues.

@Strangenoise with your example 'fix' code I assume it'll completely stop formatting the full variable completely instead of only skipping the invalid ones within the string. Is that intended behavior?

[Strangenoise]

@BigRoy a verbosity level is usually a good thing, so using ACRE_VERBOSE = 1 seems a good solution to be less silent than my proposal.

It's the purposed behavior yes, is it the best one, perhaps not.
But after all, if one part of the string can't be formatted, the whole content is not valid anymore no ?
So stop formatting the whole variable doesn't seems a problem to me.

Note that I tested my purposed fix, the resulting env (dict) returned by the function contains an empty string for my env var, which is good to me in my specific case.

[BigRoy]

Actually, doesn't your example fix raise a KeyError?
You're accessing data[key] which you have just popped prior to it?

[antirotor]

I think that solution with the ACRE_VERBOSE is fine. Or maybe we could introduce something like ACRE_STRICT=1 that will cause acre to fail when it cannot format and it will be by default on to keep the current behaviour, but we could disable strict mode on demand.

[Strangenoise]

Yes it should raise a KeyError, but instead I just got an empty string... Perhaps data is updated somewhere before the error, but I can't find where. Anyway, just logging that this specific key can't be processed can be a good solution do me.

I also find the ACRE_VERBOSE solution elegant. And as I'm working with rez I can easily set my environement and manage such an env var.