CodSpeed Instrumentation Performance Report

Merging #15000 will not alter performance

_{Comparing soapy1:ensure-environment-config-has-right-types (ee1d394) with main (b4b0e8d)}

Summary

✅ 21 untouched benchmarks

pre-commit.ci autofix

pre-commit.ci autofix

This is a nice improvement, thanks! I double checked the field types of the EnvironmentConfig and compared them to corresponding attributes of the context object in my local dev environment, and they (nearly) line up at this point:

                    field_name              field_type        context_field_type
0   aggressive_update_packages              tuple[str]           <class 'tuple'>
1             channel_priority  ChannelPriority | None  <enum 'ChannelPriority'>
2                     channels              tuple[str]           <class 'tuple'>
3             channel_settings   tuple[dict[str, str]]           <class 'tuple'>
4                deps_modifier     DepsModifier | None     <enum 'DepsModifier'>
5          disallowed_packages              tuple[str]           <class 'tuple'>
6              pinned_packages              tuple[str]           <class 'tuple'>
7                 repodata_fns              tuple[str]           <class 'tuple'>
8                   sat_solver  SatSolverChoice | None  <enum 'SatSolverChoice'>
9                       solver              str | None             <class 'str'>
10              track_features              tuple[str]           <class 'tuple'>
11             update_modifier   UpdateModifier | None   <enum 'UpdateModifier'>
12            use_only_tar_bz2             bool | None        <class 'NoneType'>

There are a few remaining incompatibilities, for example the enum types in the context do not support None as a valid setting, but maybe that's best handled in a separate issue, or maybe not at all? I guess it depends on what purpose the EnvironmentConfig is really supposed to serve, and what role if any the context has in relation to it.

Nice @peytondmurray, thanks this is really helpful!

for example the enum types in the context do not support None as a valid setting,

I think in this case it's ok for EnvironmentConfig to be different from the context. For example, if these enum values can not be None in the EnvironmentConfig then they need to always be populated with something valid. This can either be (1) the default value for the config, or (2) a null value.

The EnvironmentConfig object is meant to be created by environment spec plugins. It's valid for an environment spec to not support all the possible environment config fields. In this case, I think these plugin authors shouldn't have the responsibility of choosing good defaults for these pieces of config that they don't want to control. It's maybe best for default values to always be coming from the same source. In this case, the context object is a good source of truth for good default values.

Definitely open to looking at this another way!