Skip to content

Improve handling of Init's config arguments#1368

Closed
aphedges wants to merge 1 commit intodeepspeedai:masterfrom
aphedges:fix-init-config
Closed

Improve handling of Init's config arguments#1368
aphedges wants to merge 1 commit intodeepspeedai:masterfrom
aphedges:fix-init-config

Conversation

@aphedges
Copy link
Contributor

@aphedges aphedges commented Sep 15, 2021

Changes

  • Remove remaining usage of config instead of config_dict_or_path
  • Add note in docstring that config is deprecated
  • Fix typo in docstring

Explanation

I figured out that when using transformers, as of DeepSpeed commit e08c239, I was having issues loading models for training as fp32. I figured out that transformers is still using the config parameter, even though it does not function the way it did back before the commit. This causes InsertPostInitMethodToModuleSubClasses.dtype to default to torch.half, even though the class should take ds_config.fp16_enabled into account.

My fix here is to treat config as a deprecated alias for the new config_dict_or_path parameter. This seems reasonable given a comment on another PR: #1271 (comment). In addition, from #1008 and #1271, it seems that there only needs to be one instance of DeepSpeedConfig in the method, so I removed the second one.

I added a runtime warning so users can see that the parameter that they were using should be changed. I will be putting a PR out to transfer to the new parameter in transformers after this PR is merged and released. I'm guessing that the old config parameter should be removed at some point in the future, but I have no clue about the deprecation policy for this library.

@aphedges
Copy link
Contributor Author

It seems that #1373 was released and merged after this, so one of the core problems this PR fixes no longer exists. However, it still fixes a related bug, so I think that it is still relevant. I've rebased to incorporate the changes from the merged PR.

@aphedges
Copy link
Contributor Author

The bug and the main documentation fix in this PR were solved when #1407 was merged. No need to include this now, then.

@aphedges aphedges closed this Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments