Adds PlatformDependent factory. #739

bfloch · 2019-09-09T18:05:59Z

EDIT: See last comments for updated version for review.

Allow to specify any value in rezconfig based on platform.
Tests and documentation included.

Samples from documentation:

    from rez.utils.config import PlatformDependent
    default_shell = PlatformDependent({
        "linux": "bash",
        "windows": "powershell",
        "*": "zsh",
    })

* can be used as fallback key.
By default it matches the platform_.name. This can be changed by specifying
attr as any other platform_ attribute:

    from rez.utils.config import PlatformDependent
    default_shell = PlatformDependent({
        "AMD64": "bash",
        "x86": "powershell",
        "*": "zsh",
    },
    attr="arch")

I haven't tried but this could be even chained for more complex rules.

Backwards compatibility

This is just a factory. All schema validation still works as proven by the unit tests
No breaks in compatibility. This is just an option, it is not used by default.

nerdvegas · 2019-09-11T00:03:11Z

Hey Blazej,

Fantastic idea, I like it. I can see how this would simplify setting definitions with nested dicts. I have some suggestions though:

I think it's a little confusing to use PlatformDependent, but then specify that it's actually (eg) arch dependent by passing attr='arch'. Also, having to know the available attributes on platform_ is a little too low level. This is just the config, it needs to be pretty basic to use. To that end, I think it'd be good to have have multiple classes instead - PlatformDependent, ArchDependent etc.
You should not have to import the class in rezconfig to use it. We should make it available already, as is done with ModifyList. See https://github.com/nerdvegas/rez/blob/master/src/rez/config.py#L814
I think it'd be better to pass the default value as an arg, rather than as '*' in the map arg.
Lastly, I think we're missing a trick here, this could be more useful as a more general solution! For example:

# in a rezconfig.py
default_shell = Conditional(
    value=os.getenv("STUDIO_LOCATION"),
    default="bash",
    options={
      "LA": "bash",
      "Paris": "zsh"
    }
)

We would still have PlatformDependent etc, but that would just be a specific case of Conditional.

bfloch · 2019-09-11T00:51:41Z

Very glad you like it. I also had a good feeling about it.
All your points are valid. Will look into it tomorrow.

bfloch · 2019-09-11T19:31:05Z

First commit is a bugfix in the current override that I found along the way.
Let me know if you prefer a separate PR.
& 4. Implemented a Conditional generic version and the derived PlatformDependent, OsDependent and ArchDependent forms. The latter two do not respect platform_map as this would mean a recursion. But I guess we can live with that.
These guys are built-ins for rezconfig now.
"*" is now a default argument. I had to actually default it to the exception type to raise so that it works with None. This also means that you can control the exception in case this factory is used in other contexts.

+Added tests for using in a config file. This exposed the recursive import mentioned above.

bfloch · 2019-09-13T14:55:44Z

I was thinking about what would be necessary to make OsDependent and ArchDependent respect platform_map.
It would seem that the platform_map would need to load a config in a special mode where only the platform_map key is being extracted, without the implicit conditionals being bound.
Not sure if this is doable without making a mess but I'll take a look at it.

bfloch · 2019-09-16T21:52:53Z

This is doing what one would expect.

However due to the nature of platform_map I need to use inspect.
It is still possible (and necessary in the case of config.override) to specify the platform_map explicitly for OsDependent and ArchDependent, but it was important to me that it would just work within a regular config and still use the configs current platform_map (if there is any).

All the different types are visible in the unit tests.

Also removed the platform_mapped decorator which make the code easier.

bfloch · 2019-09-17T00:54:13Z

Last thing I might do is use the inspect only in the bound class. This way there shouldn't be any wrong results.

bfloch · 2019-09-17T14:22:21Z

Ok this is much better. Ignore the past discussion.
Here an updated descriptions:

Implemented a generic Conditional version and the derived PlatformDependent, OsDependent and ArchDependent forms.
The conditionals are built-ins for rezconfig. In order to respect the current config's platform_map, a special InConfig*-variants of the Conditional is used (but exposed under the same name, so their interface matches).
This means that the conditionals will respect the platform_map in a case like this:

# This is a rezconfig.py

platform_map = {
    "arch": {
        ".*": "IMPOSSIBLE_ARCH",
    },
}

prune_failed_graph = ArchDependent({
    "IMPOSSIBLE_ARCH": True,
})

In the case of an config.override you need to make sure to pass the correct platform_map instead (if you require it). The example above would be something like this:

config.override(
    "prune_failed_graph",
    ArchDependent(
        {
            "IMPOSSIBLE_ARCH": True,
        },
        platform_map=config.platform_map
    )
)

You can pass a default. It can be an exception (which it defaults to) which allows None defaults to work. This also means that you can control the exception in case this factory is used in other contexts. 👍
Last commit is a bugfix in the current override that I found along the way (plugins would be reset if a non-plugin override came after a plugin override).
Let me know if you prefer a separate PR.
Includes tests.

EDIT:

Replaced platform_mapped decorator with a method.

bfloch · 2019-09-17T14:34:10Z

maxnbk · 2019-10-04T03:23:28Z

I have some failing tests on Windows in this branch (on py3.7), is this the right place for those? Granted, it's far fewer failing tests than is currently in master, so that doesn't have to be a blocker.

src/rez/config.py

nerdvegas · 2019-10-04T04:03:27Z

So there's a problem I can see that I think has to be fixed.

While Os/ArchDependent does respect platform_map, it only does so if platform_map is defined in the same config. The problem with this is, it would not be at all unreasonable for a studio to define platform_map in some root rezconfig.py, and then have various overrides in a second rezconfig.py (you can specify multiple, and they get merged together). In that scenario, any OsDependent used in the second rezconfig would not work as expected.

I think I can see a reasonable fix though - it isn't perfect, but good enough I reckon. What we need to do is, in _load_config_from_filepaths, make the config data read so far, available in some reserved variable - the same way that's done in _load_config_py. Then, InspectedDependent would have access to both platform_map if defined in the current config, but also if defined in an earlier config.

I think this should be a fairly trivial fix. It's not perfect in the sense that OsDependent won't be aware of platform_map being redefined in a config later in the searchpath - but I think the chances of that being done are low, and it's not that unreasonable for OsDependent to only know about platform_map as it's been defined up to that point in the config searchpath.

ps - We could also provide a reserved get_config_value function, which does the inspecting of current config and/or previous merged configs, to get the value you're after. I think this would make sense because we could see this pattern again in a different case, and it'd keep all the messy details (frame introspection etc) in the one place.

nerdvegas

See last comment

src/rez/tests/test_config.py

bfloch · 2019-10-04T16:14:59Z

I have some failing tests on Windows in this branch (on py3.7), is this the right place for those? Granted, it's far fewer failing tests than is currently in master, so that doesn't have to be a blocker.

The only tests that are expected to fail are the cmake related ones.
I will check Python 3.7. Thanks for the hint.

src/rez/utils/platform_.py

maxnbk · 2019-10-05T21:28:06Z

These are my other (non-py3-specific) test case failures at the moment.
I apologize if any of these are caused by my specific Win10 home system. It doesn't get as much love as it should. ;)

======================================================================
ERROR: test_conditional_in_file (rez.tests.test_config.TestConfig)
Test package config overrides.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 628, in __new__
    return options[key]
KeyError: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 846, in _load_config_py
    exec(code, g)
  File "c:\opt\rez\lib\site-packages\rez\tests\data\config\test_conditional.py", line 19, in <module>
    "IMPOSSIBLE_ARCH": True,
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 700, in __new__
    default)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 689, in __new__
    return base(options, default, platform_map=platform_map)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 659, in __new__
    default=default)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 631, in __new__
    raise default(*e.args)
rez.exceptions.ConditionalConfigurationError: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH' not found in Conditional options

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_config.py", line 137, in test_conditional_in_file
    self.assertListEqual(c.plugins.release_hook.emailer.recipients, ["joe@here.com"])
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 535, in plugins
    plugin_data = self._data.get("plugins", {})
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 623, in _data
    data = copy.deepcopy(self._data_without_overrides)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 618, in _data_without_overrides
    data, self._sourced_filepaths = _load_config_from_filepaths(self.filepaths)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 893, in _load_config_from_filepaths
    data_ = loader(filepath_with_ext)
  File "c:\opt\rez\lib\site-packages\rez\backport\lru_cache.py", line 96, in wrapper
    result = user_function(*args, **kwds)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 849, in _load_config_py
    % (filepath, str(e)))
rez.exceptions.ConfigurationError: Error loading configuration from c:\opt\rez\lib\site-packages\rez\tests\data\config\test_conditional.py: 'IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH' not found in Conditional options

======================================================================
ERROR: test_conditional_overrides (rez.tests.test_config.TestConfig)
Test configuration with platform conditionals
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 53, in validate
    data = self.schema.validate(data)
  File "c:\opt\rez\lib\site-packages\rez\vendor\schema\schema.py", line 230, in validate
    raise SchemaError('%r should be instance of %r' % (data, s), e)
rez.vendor.schema.schema.SchemaError: 'Wrong Type' should be instance of <class 'bool'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_config.py", line 94, in test_conditional_overrides
    platform_name: True,
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 486, in override
    self._uncache(key, keep_plugins=True)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 589, in _uncache
    if key and hasattr(self, key):
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 192, in __get__
    result = self.func(instance)
  File "c:\opt\rez\lib\site-packages\rez\utils\data_utils.py", line 588, in getter
    return self._validate_key(key, attr, key_schema)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 614, in _validate_key
    return key_schema.validate(value)
  File "c:\opt\rez\lib\site-packages\rez\config.py", line 57, in validate
    % (self.key, str(e)))
rez.exceptions.ConfigurationError: Misconfigured setting 'debug_none': 'Wrong Type' should be instance of <class 'bool'>

======================================================================
FAIL: test_execute_command_environ (rez.tests.test_context.TestContext)
Test that execute_command properly sets environ dict.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\opt\rez\lib\site-packages\rez\tests\test_context.py", line 82, in test_execute_command_environ
    self.assertEqual(parts, ["covfefe", "hello"])
AssertionError: Lists differ: [''] != ['covfefe', 'hello']

First differing element 0:
''
'covfefe'

Second list contains 1 additional elements.
First extra element 1:
'hello'

- ['']
+ ['covfefe', 'hello']

bfloch · 2019-10-31T18:13:50Z

These are my other (non-py3-specific) test case failures at the moment.
I apologize if any of these are caused by my specific Win10 home system. It doesn't get as much love as it should. ;)

...

I think this one is not related to this PR but we can be sure by holding this one off until the Windows tests PR changes are in: #775 #781

I'll be looking at Allan's requests next.

bfloch · 2019-11-04T20:14:46Z

Added the suggested Python3 change
Fixed the bug of repeated keys in the platform map (IMPOSSIBLE_ARCHIMPOSSIBLE_ARCH)
@nerdvegas Gave your suggestion a try. One detail is I had to introduce a new hashable dict in order to keep the caches. If anyone knows a nicer way to deal with this, please let me know.

If you don't mind let's have the get_config_value as a separate refactoring PR, I haven't gotten my head around how this would work with the suggested Conditional classes.

I am still trying to figure what went wrong with Python 3.x ...

bfloch · 2019-11-04T21:38:54Z

Should be in a pretty good shape now.

bfloch · 2019-11-05T19:06:28Z

@maxnbk I fixed the issues you had (I hope).
I'll open this up again for review.

bfloch · 2019-11-06T18:51:45Z

Moved the test to test_utils.

nerdvegas · 2019-11-26T05:11:47Z

Hey @bfloch, just getting back to this now, apologies for the delay.

There's only a couple of minor changes I would make. I don't mind doing them myself as part of the merge, as they're fairly trivial.

I think the fallback_platform_map stuff could be more general. Ie, the _load_config_py and _load_config_yaml files would take a merged_data arg instead, which would make data from previous configs in the config path available to the currently eval'd config (stored as reserved __merged_data for eg). This is less about generalising for use in other cases, and more about making the code easier to understand.
I would move the InspectedDependent and derived classes from data_utils.py into config.py, as I think they're too specific to the config to be elsewhere. These classes would also expect to see __merged_data in their current frame rather than __fallback_platform_map. Now I think this would be more obvious - these classes would be checking __merged_data.get("platform_map"), and we'd avoid the more special case fallback_platform_map in several places.

cheers
A

Allow to specify any value in rezconfig based on platform or any other key. Tests and documentation included.

Since the conditionals can be called within a config, and this config might have a platform_map we try to respect it best we can. In an config.override() the containing configuration can not be known so it needs to be passed explicitly.

This makes less assumptions. No need to have introduced a decorator for two functions only (original author speaking).

Before it would affect the general use of the classes so I made the particular case where these Conditionals are bound explicit classes.

…lugin override

In python 3 the caught execption is not recoverable in an override. Since we are testing for expected failure it is probably ok to not expect any tests after the failure to pass.

bfloch · 2020-09-02T05:49:34Z

Just rebased and resolved conflict with current master. I'll be looking into your feedback shortly. Sounds minimal and it seems we might be wanting to use this shortly which will also give us some real world feedback.

P.S. Nice how far all of you pushed the GH actions!

nerdvegas · 2020-09-02T05:51:19Z

Hey cheers Blazej, I'll get back onto this soon.

…

On Wed, 2 Sep. 2020, 15:49 Blazej Floch, ***@***.***> wrote: Just rebased and resolved conflict with current master. I'll be looking into your feedback shortly. Sounds minimal and it seems we might be wanting to use this shortly which will also give us some real world feedback. P.S. Nice how far all of you pushed the GH actions! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMOUSWLGEZSDAAZLEACO2TSDXMHVANCNFSM4IU6L54Q> .

bfloch mentioned this pull request Sep 9, 2019

Better Environment Variable control in Rex #737

Open

9 tasks

bfloch force-pushed the feature/platform_dependent_rezconfig branch 4 times, most recently from 3c5e673 to e5e289d Compare September 11, 2019 19:28

bfloch force-pushed the feature/platform_dependent_rezconfig branch 2 times, most recently from cb1544b to d483050 Compare September 16, 2019 21:49

bfloch force-pushed the feature/platform_dependent_rezconfig branch 2 times, most recently from d4e59e8 to 29bc81a Compare September 17, 2019 14:07

bfloch requested review from maxnbk and nerdvegas September 17, 2019 14:34

nerdvegas reviewed Oct 4, 2019

View reviewed changes

src/rez/config.py Show resolved Hide resolved

nerdvegas suggested changes Oct 4, 2019

View reviewed changes

bfloch commented Oct 4, 2019

View reviewed changes

src/rez/tests/test_config.py Show resolved Hide resolved

maxnbk reviewed Oct 5, 2019

View reviewed changes

src/rez/utils/platform_.py Outdated Show resolved Hide resolved

nerdvegas mentioned this pull request Oct 5, 2019

use subprocess in 'text' mode in most cases #753

Merged

bfloch force-pushed the feature/platform_dependent_rezconfig branch from 29bc81a to 305d47a Compare November 4, 2019 20:07

bfloch force-pushed the feature/platform_dependent_rezconfig branch from 273dd45 to c07bc4f Compare November 4, 2019 21:32

bfloch requested a review from nerdvegas November 4, 2019 21:38

bfloch force-pushed the feature/platform_dependent_rezconfig branch from c07bc4f to 2665518 Compare November 5, 2019 18:55

bfloch requested a review from maxnbk November 5, 2019 19:06

nerdvegas mentioned this pull request Nov 5, 2019

add get_config_value util func in config.py #784

Open

bfloch force-pushed the feature/platform_dependent_rezconfig branch from 2665518 to 2b3ceeb Compare November 6, 2019 18:50

bfloch and others added 9 commits September 2, 2020 01:30

Adds Conditional factories for rezconfig.

6e854e6

Allow to specify any value in rezconfig based on platform or any other key. Tests and documentation included.

Replaced platform_mapped decorator.

987e849

This makes less assumptions. No need to have introduced a decorator for two functions only (original author speaking).

Makes in-config Os- and ArchDepedent inspection explicit to this case.

0f25979

Before it would affect the general use of the classes so I made the particular case where these Conditionals are bound explicit classes.

Fixes unnecassary config.plugins clearing if override happens after p…

cd75ac5

…lugin override

Python3

29be01a

Fix repetition of keys in platform_map

15d0148

Take previous config platform_map into account.

045e553

Fixes Python3 tests

2b5096f

In python 3 the caught execption is not recoverable in an override. Since we are testing for expected failure it is probably ok to not expect any tests after the failure to pass.

bfloch force-pushed the feature/platform_dependent_rezconfig branch from 2b3ceeb to 2b5096f Compare September 2, 2020 05:34

maxnbk requested a review from a team as a code owner November 15, 2022 23:58

JeanChristopheMorinPerso mentioned this pull request Dec 3, 2022

shebang and executable paths incorrect in gitbash #1302

Open

JeanChristopheMorinPerso requested a review from a team as a code owner April 4, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds PlatformDependent factory. #739

Adds PlatformDependent factory. #739

bfloch commented Sep 9, 2019 •

edited

Loading

nerdvegas commented Sep 11, 2019

bfloch commented Sep 11, 2019

bfloch commented Sep 11, 2019

bfloch commented Sep 13, 2019

bfloch commented Sep 16, 2019

bfloch commented Sep 17, 2019

bfloch commented Sep 17, 2019 •

edited

Loading

bfloch commented Sep 17, 2019

maxnbk commented Oct 4, 2019 •

edited

Loading

nerdvegas commented Oct 4, 2019 •

edited

Loading

nerdvegas left a comment

bfloch commented Oct 4, 2019

maxnbk commented Oct 5, 2019 •

edited

Loading

bfloch commented Oct 31, 2019

bfloch commented Nov 4, 2019

bfloch commented Nov 4, 2019

bfloch commented Nov 5, 2019

bfloch commented Nov 6, 2019

nerdvegas commented Nov 26, 2019

bfloch commented Sep 2, 2020

nerdvegas commented Sep 2, 2020 via email

Adds PlatformDependent factory. #739

Are you sure you want to change the base?

Adds PlatformDependent factory. #739

Conversation

bfloch commented Sep 9, 2019 • edited Loading

Backwards compatibility

nerdvegas commented Sep 11, 2019

bfloch commented Sep 11, 2019

bfloch commented Sep 11, 2019

bfloch commented Sep 13, 2019

bfloch commented Sep 16, 2019

bfloch commented Sep 17, 2019

bfloch commented Sep 17, 2019 • edited Loading

bfloch commented Sep 17, 2019

Tests passing

maxnbk commented Oct 4, 2019 • edited Loading

nerdvegas commented Oct 4, 2019 • edited Loading

nerdvegas left a comment

Choose a reason for hiding this comment

bfloch commented Oct 4, 2019

maxnbk commented Oct 5, 2019 • edited Loading

bfloch commented Oct 31, 2019

bfloch commented Nov 4, 2019

bfloch commented Nov 4, 2019

bfloch commented Nov 5, 2019

bfloch commented Nov 6, 2019

nerdvegas commented Nov 26, 2019

bfloch commented Sep 2, 2020

nerdvegas commented Sep 2, 2020 via email

bfloch commented Sep 9, 2019 •

edited

Loading

bfloch commented Sep 17, 2019 •

edited

Loading

maxnbk commented Oct 4, 2019 •

edited

Loading

nerdvegas commented Oct 4, 2019 •

edited

Loading

maxnbk commented Oct 5, 2019 •

edited

Loading