Refactor/Pydantify comms config

[mrwyattii]

Splitting work from #2210 to make it easier to review and merge.

Comms already used Pydantic, but updating it to use the DeepSpeedConfigModel like all other configs

TODO:
- [ ] Refactor how the config is used

• Verify that this code is tested somehow in unit tests
• Add auto-gen docs

[mrwyattii]

@Quentin-Anthony could you take a look at this PR? Also, do we have any unit tests that cover the comm logger? I couldn't find any and I'm wondering if you might be able to add some or help me add some?

[Quentin-Anthony]

Yeah I can take a look.

One immediate feedback is that 3bff0be will still fail because of the other checks in https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L82 will fail here: https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L97-L107

Lemme push a quick commit that replaces these assignments with setattr() calls in case the attr doesn't exist. That way we can do away with the __init__() func in class CommsLogger as you intended.

[Quentin-Anthony]

There are no unit tests for the comms logger. I'll create some and commit them to this PR.

[mrwyattii]

Yeah I can take a look.

One immediate feedback is that 3bff0be will still fail because of the other checks in

https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L82

will fail here:
https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L97-L107

Lemme push a quick commit that replaces these assignments with setattr() calls in case the attr doesn't exist. That way we can do away with the __init__() func in class CommsLogger as you intended.

I'm not sure I follow. The code block you linked to should create those attributes if they don't already exist. I think defining self.enabled in the CommsLogger.__init__() is the only one necessary.

[Quentin-Anthony]

Yeah I can take a look.
One immediate feedback is that 3bff0be will still fail because of the other checks in
https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L82

will fail here:
https://github.com/microsoft/DeepSpeed/blob/841ee593b943de4fd14db7d352d5bcb6b25f2ff9/deepspeed/comm/comm.py#L97-L107

Lemme push a quick commit that replaces these assignments with setattr() calls in case the attr doesn't exist. That way we can do away with the __init__() func in class CommsLogger as you intended.

I'm not sure I follow. The code block you linked to should create those attributes if they don't already exist. I think defining self.enabled in the CommsLogger.__init__() is the only one necessary.

You're right. I misread the error log and got very confused, looks like. Ignore my comment :)

[mrwyattii]

There are no unit tests for the comms logger. I'll create some and commit them to this PR.

@Quentin-Anthony Are you still able to do this? If not, I can put something together and get your input.

[Quentin-Anthony]

There are no unit tests for the comms logger. I'll create some and commit them to this PR.

@Quentin-Anthony Are you still able to do this? If not, I can put something together and get your input.

My bad! This slipped my mind. Working on it now.

[Quentin-Anthony]

I have tests in mrwyattii#1. Let me know if you have any comments :)

[Quentin-Anthony]

@mrwyattii -- Anything else needed here?

[mrwyattii]

@mrwyattii -- Anything else needed here?

Thanks @Quentin-Anthony! These look great