Change functional surgery method return values to None

[nik-mosaic]

Closes https://mosaicml.atlassian.net/browse/CO-988 and #1441 .
For consistency sake, and to ensure that the user knows the model is being mutated, all applicable functional model surgery methods return None, rather than some returning the model object and some returning None.

[mvpatel2000]

Can we instead return how many layers are changed? Would be very useful for agent bc then it can skip surgeries if they are no-ops

[nik-mosaic]

This is an option. However, there are lots of algorithms which aren't surgery based, for which there is no clear number to return. e.g.: compute_ema. One priority should be consistency for users using the functional interface. Returning non-none will reduce this.

We currently raise NoEffectWarnings for no-op model surgery algorithms; the agent could catch those warnings.

[hanlint]

Agreed with @nik-mosaic , having the methods return an int would lead to odd bugs when users try:

model = apply_x(model)

whereas None would clearly indicate that this is an in-place operation.

[dskhudia]

IMO it's ok to return "something" for apply methods if it makes sense (e.g., number of layers replaced as @mvpatel2000 suggested) but not model if apply method is modifying the model inplace. Any user of apply_* method will look at least look at the API to see what it does and will be able to see return type/value. Therefore, consistency across the board for the apply APIs is, I think, too restrictive.

[mvpatel2000]

Agreed with @nik-mosaic , having the methods return an int would lead to odd bugs when users try:

model = apply_x(model)

whereas None would clearly indicate that this is an in-place operation.

I think returning an int would pretty clearly indicate what's going on here, and the docs can be clear this is in-place. It's a lot cleaner to just measure how many layers are changed vs. trying to catch warnings and interpret them.

Basically +1 to Daya's points

[nik-mosaic]

Per offline discussion, functional model surgery methods will return # of modified layers where applicable and None elsewhere.

[dskhudia]

This may break code that uses functional version of algorithms and uses return values. I think that's ok though.

[hanlint]

LGTM, one request:

Add a note here (https://docs.mosaicml.com/en/v0.10.1/trainer/algorithms.html#model-surgery) explaining that all surgery methods return the number of instances replaced.

Also, do we need to add a test to enforce this API?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[hanlint]

@nik-mosaic do we close this PR?

[nik-mosaic]

@nik-mosaic do we close this PR?

In a December design discussion, we backtracked on our decision to return the number of layers modified. We decided that for user-friendliness, functional algorithms should return None instead. This PR is now ready to merge.

[mvpatel2000]

Should go out on 13. Holding off for now since we're going to cut a 12.1 off dev

[mvpatel2000]

Good to merge