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

Look into supporting Union in Structured configs #144

Open
omry opened this issue Jan 30, 2020 · 15 comments
Open

Look into supporting Union in Structured configs #144

omry opened this issue Jan 30, 2020 · 15 comments
Assignees
Labels
enhancement New feature or request priority_high
Milestone

Comments

@omry
Copy link
Owner

omry commented Jan 30, 2020

No description provided.

@omry omry added this to the OmegaConf 2.0.0 milestone Jan 30, 2020
@omry omry modified the milestones: OmegaConf 2.0, OmegaConf 2.1 Mar 6, 2020
@omry omry added the enhancement New feature or request label Mar 24, 2020
apsdehal added a commit to apsdehal/mmf that referenced this issue Sep 16, 2020
Summary:
As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method for MMBT

There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Differential Revision: D23699688

fbshipit-source-id: 0cd18791e2cbce454f40088dca4b35443b62b567
apsdehal added a commit to apsdehal/mmf that referenced this issue Sep 22, 2020
Summary:
Pull Request resolved: facebookresearch#565

As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method for MMBT

There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Differential Revision: D23699688

fbshipit-source-id: eb98cec2a8464bb41968543c36e8c0e1e469de86
apsdehal added a commit to apsdehal/mmf that referenced this issue Oct 7, 2020
Summary:
Pull Request resolved: facebookresearch#565

As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method for MMBT
- Updates build method to support passing of direct config object
- Add Config class to BaseModel as well and update typings
There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Differential Revision: D23699688

fbshipit-source-id: 3f0f43938814a1c6bfdef51c048bc7ee8ef2c28b
apsdehal added a commit to apsdehal/mmf that referenced this issue Oct 12, 2020
Summary:
Pull Request resolved: facebookresearch#565

As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method to the BaseModel class so that args initialization is possible
- Updates build method to support passing of direct config object
- Add Config class to BaseModel as well and update typings
There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Reviewed By: vedanuj

Differential Revision: D23699688

fbshipit-source-id: 4f59e1c7b61069e7ae266c76b9afcf59edaaa706
apsdehal added a commit to apsdehal/mmf that referenced this issue Oct 12, 2020
Summary:
Pull Request resolved: facebookresearch#565

As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method to the BaseModel class so that args initialization is possible
- Updates build method to support passing of direct config object
- Add Config class to BaseModel as well and update typings
There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Differential Revision: D23699688

fbshipit-source-id: 29f738d82c617c528212118ddbb60ee89bfca442
facebook-github-bot pushed a commit to facebookresearch/mmf that referenced this issue Oct 12, 2020
Summary:
Pull Request resolved: #565

As step one of FAIM integration, we allow building our models from config so that the models are purely decoupled from configuration and users know what the model expects.

We first do this for the MMBT model.

- Adds configs for MMBT and respective encoders.
- Also adds from_params method to the BaseModel class so that args initialization is possible
- Updates build method to support passing of direct config object
- Add Config class to BaseModel as well and update typings
There is an issue with OmegaConf that doesn't let us use Union in structured configs. Take a look at omry/omegaconf#144

Reviewed By: vedanuj

Differential Revision: D23699688

fbshipit-source-id: 37020346ce820207eb41b7bd43c8fba579b436d5
@omry omry removed the In progress label Feb 17, 2021
@omry
Copy link
Owner Author

omry commented Feb 22, 2021

Bouncing to 2.2.

@jieru-hu
Copy link
Collaborator

comments from receipe team:

Does this Union support include the new syntax being introduced (eg. TypeA | TypeB) ?

@kandluis
Copy link

kandluis commented Dec 17, 2021

For context, this is the proposal (https://www.python.org/dev/peps/pep-0604/), which is introduced in Python 3.10 (which is now also made available at Meta).

It's a big readability win. I'd say it's of middle-to-low importance for us, but just checking if it'll work with the support here in hydra. I suspect so since I assume Union[A, B, ...] and A | B | ... will translate to the same underlying code, so might be entirely transparent to you.

Thanks for thinking of this!

@Jasha10
Copy link
Collaborator

Jasha10 commented Dec 17, 2021

Yes, it should be possible to support PEP 604 in structured config type hints.

@pixelb pixelb modified the milestones: OmegaConf 2.2, OmegaConf 2.3 May 18, 2022
luator added a commit to machines-in-motion/breathing-cat that referenced this issue Nov 11, 2022
NOTE: This is currently blocked as OmegaConf does not yet fully support
Union in the types.  This is on the roadmap for 2.3 though:
omry/omegaconf#144
@peterdavidfagan
Copy link

peterdavidfagan commented Feb 14, 2024

Does there exist a workaround for this issue? I have model parameters like kernel dimension which have type Union[int, Sequence[int]] that is causing issues for omegaconf.

encoder:
  _target_: multi_modal_transformers.tokenizers.images.image_tokenizer.ImageTokenizer
  image_size: [280, 280, 3]
  patch_size: 56
  normalize: True # normalizes the input image pixel values to range [-1, 1]
  position_interval: 128 # number of discrete values for encoding patch positions
  rng_collection: patch_encoding # pseudo-random number parameter name
  embedding_dim: 512

  # positional embedding layers

  row_position_embedding:
    _target_: flax.linen.Embed
    name: "image_row_position_embedding"
    num_embeddings: ${tokenizers.images.encoder.position_interval}
    features: 512
    dtype: ${dtype}
    param_dtype: ${param_dtype}
    embedding_init:
      _target_: flax.linen.initializers.variance_scaling
      scale: 1.0
      mode: "fan_in"
      distribution: "normal"
      dtype: ${dtype}

  col_position_embedding:
    _target_: flax.linen.Embed
    name: "image_col_position_embedding"
    num_embeddings: ${tokenizers.images.encoder.position_interval}
    features: 512
    dtype: ${dtype}
    param_dtype: ${param_dtype}
    embedding_init:
      _target_: flax.linen.initializers.variance_scaling
      scale: 1.0
      mode: "fan_in"
      distribution: "normal"
      dtype: ${dtype}

  # input projection layer
  resnet:
    _target_: multi_modal_transformers.tokenizers.images.image_tokenizer.ResNetV2Block
    num_blocks: 2
    input_conv:
      _target_: flax.linen.Conv
      features: 64
      kernel_size: [12, 12]
      strides: [2, 2]
      padding: VALID
      use_bias: True
      dtype: ${dtype}
      param_dtype: ${param_dtype}
      kernel_init:
        _target_: flax.linen.initializers.he_normal
        dtype: ${param_dtype}
      bias_init:
        _target_: flax.linen.initializers.normal
        dtype: ${param_dtype}

    input_pool:
      _partial_: true
      _target_: flax.linen.max_pool
      window_shape: [3,3]
      strides: [1,1]
      padding: VALID

    # resnet blocks
    resnet_norm:
      _target_: flax.linen.GroupNorm
      num_groups: 32
      epsilon: 1e-6
      dtype: ${dtype}
      param_dtype: ${param_dtype}

    resnet_activation:
      _partial_: true
      _target_: flax.linen.gelu

    resnet_conv:
      _target_: flax.linen.Conv
      features: 64
      kernel_size: [3,3]
      strides: [1,1]
      padding: SAME
      use_bias: True
      dtype: ${dtype}
      param_dtype: ${param_dtype}
      kernel_init:
        _target_: flax.linen.initializers.he_normal
        dtype: ${param_dtype}
      bias_init:
        _target_: flax.linen.initializers.normal
        dtype: ${param_dtype}

    # output_layer
    output_dense:
      _target_: flax.linen.Dense
      features: 512
      kernel_init:
        _target_: flax.linen.initializers.he_normal                                                                                                                                                            
        dtype: ${param_dtype}                                                                                                                                                                                  
      bias_init:                                                                                                                                                                                               
        _target_: flax.linen.initializers.normal                                                                                                                                                               
        dtype: ${param_dtype}

Error:

omegaconf.errors.ConfigValueError: Unions of containers are not supported:
strides: Union[int, Sequence[int]]
    full_key: 
    object_type=None

I seem to get this when I have nested configuration files containing _target_ methods with Unions.

@odelalleau
Copy link
Collaborator

Does there exist a workaround for this issue?

Using Any for typing should work.

@peterdavidfagan
Copy link

peterdavidfagan commented Feb 14, 2024

Using Any for typing should work.

Thanks @odelalleau, makes sense. I guess I was hoping I might be able to do something that wouldn't involve editing the function signatures of a dependent library, I could do so but I wanted to understand if I could do something with OmegaConf to circumvent this error.

I've added correct and relaxed (Any) type annotations in my own code (where relaxed version is for testing purposes) but it looks like the underlying libraries I am using are causing the issue. For instance the below module is instantiated in my architecture, it contains modules with types that are Unions (e.g. conv).

class ResNetV2Block(nn.Module):
    num_blocks: Any
    # input projection layer
    input_conv: Any
    input_pool: Any
    # resnet blocks
    resnet_norm: Any
    resnet_activation: Any
    resnet_conv: Any
    # output_layer
    output_dense: Any

    @nn.compact
    def __call__(self, x):
        # start with convolution projection
        x = instantiate(self.input_conv)(x)
        x = call(self.input_pool)(x)

        # resnetv2block
        residual = x
        
        for _ in range(self.num_blocks):
            x = instantiate(self.resnet_norm)(x)
            x = call(self.resnet_activation)(x)
            x = instantiate(self.resnet_conv)(x)

        if residual.shape != x.shape:
            residual = instantiate(self.resnet_conv)(residual)
  
        x = x+residual
        
        #flatten output
        x = jnp.reshape(x, (*x.shape[:3], -1))
        x = instantiate(self.output_dense)(x)

        return x

This resnet block is instantiated as the tokenizer within a larger architecture which I wanted to manage with Hydra.

@odelalleau
Copy link
Collaborator

@peterdavidfagan You need to change it in the structured config definition (the dataclass).

@peterdavidfagan
Copy link

peterdavidfagan commented Feb 14, 2024

@peterdavidfagan You need to change it in the structured config definition (the dataclass).

Ah ok I am still a bit of a hydra noob, I'll reread the docs and update my resolution here for reference. I have simply been reading my config yaml as follows and not interacting directly with the structured config definition (starting to read up on this now):

hydra.core.global_hydra.GlobalHydra.instance().clear()
initialize(version_base=None, config_path="../../model_configs", job_name="gato")
GATO_CONFIG = compose(
        config_name="gato_base",
        )

Ah ok the docs are super helpful. Thanks @odelalleau your response helped get me on the right track, I probably should have consulted the docs on structured configs in greater depth.

@odelalleau
Copy link
Collaborator

@peterdavidfagan glad if I could help, but from what you shared it looks like you may not actually be using structured configs since you're just using the compose API with yaml files (unless there is some other code -- maybe triggered when importing another library -- that would define the structured config in a dataclass?)

@peterdavidfagan
Copy link

peterdavidfagan commented Feb 15, 2024

Thanks @odelalleau, in the end I fixed the bug in my code. I did try to manually alter the configstore before composing my configs where I would specify the types for various fields in a dataclass definition, unfortunately this didn't work but likely due to my bug being caused by something else.

What was happening in my case was that the _recursive_ flag in one of my instantiate methods was set to True rather than False. Rather than passing a DictConfig to subsequent calls to instantiate my code was passing the instantiated layer, having recursive set to true resulted in the error I was getting.

Super happy to have refactored my hydra config, it is now making my code very succinct and easy to dynamically configure different versions of my architecture.

@mshvartsman
Copy link

Are there any updates on this functionality? It would be very useful for cases like the following:

@dataclass
class TrainingConfig:
    optimizer_config: SGDConfig | AdamConfig

(where different optimizers might have different configs). I see #913 mentions that:

Unions of containers (e.g. Union[List[str], Dict[str, str], MyStructuredConfig]) are not yet supported. This will come in a followup PR.

but I have not seen any other updates.

@alexdremov
Copy link

Are there updates on this?

@Jasha10 @omry

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 3, 2024

@alexdremov no updates at this time. Currently OmegaConf is being maintained by volunteers -- we would need someone to take on the implementation and write tests.

@alexdremov
Copy link

@alexdremov no updates at this time. Currently OmegaConf is being maintained by volunteers -- we would need someone to take on the implementation and write tests.

Got it. Thanks for your reply!

Maybe I'll try to dig into it soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority_high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants