[JAX] Fix missing axes parameters in TransformerLayer and the wrong shape of bias in LayerNormMLP

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?

[mingxu1067]

• Fixed typo, bais to bias.
• Fixed the wrong shape of bias in LayerNormMLP when enable gated activations, such as ('gelu', 'linear').

For example, if we apply ('gelu', 'linear'), then the shape of bias should be ('num_act', 'hidden'), that is (2, hidden) in this case, but we always generate (1, hidden), then led to incorrect results when enable_bias=True.

• Fixed the issue of misshing bias_axes parameter passed to sub-layers in TransformerLayer, which cause incorrect PSpec in sharding when running with mutli-devices.

[mingxu1067]

/te-ci

[nouiz]

What was the consequence of the bug? Crash? Wrong value? The graph wasn't what the user expected, but it was giving reasonable results?

[timmoon10]

Seems reasonable, but I agree more explanation of what's going on would be helpful.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]. If this is always the case, could we just remove the requirement for the user to provide bias_axes and reduce the risk of user error? I'm not sure of the implications, but perhaps something for a future PR.

[mingxu1067]

Update description for the clear explanation.

It seems with our current interface for DenseGeneral-like modules, we expect the user to set kernel_axes and also bias_axes=kernel_axes[1:]

This makes sense. It always equals to kernel_axes[1:]. Considering to remove bias_axes in the next PR.

[mingxu1067]

@timmoon10 Are there remaining issues on this PR? if not, could we merge this into main?