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

Cm3 integration #727

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Cm3 integration #727

wants to merge 25 commits into from

Conversation

urielsinger
Copy link
Contributor

No description provided.

@ArmenAg
Copy link
Contributor

ArmenAg commented Jun 6, 2023

This has some specific logic to CM3leon project (i.e., img token conversions), are we sure we want to land this in main? Do we want to possible pick a branch and merge everything into there and periodically update to main?

process_group=distributed_utils.get_data_parallel_group(),
)
model = task.build_model(cfg.model)
if not isinstance(model, FullyShardedDataParallel):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, this is for loading up consolidated model for training?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
I added support to change the MP size during job lunch, and for that I need to wrap it in FullyShardedDataParallel inside the build_model.
As I don't want to double wrap it, I needed to add this if..

@@ -4,6 +4,7 @@
# LICENSE file in the root directory of this source tree.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are changes to the cm3 objectives that i landed in scaling_racm3 correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly.

@ArmenAg
Copy link
Contributor

ArmenAg commented Jun 6, 2023

Code LGTM. @suchenzang to give guidance on whether or not to land here.

@@ -246,6 +281,10 @@ def _check_cm3_parameterization(self):

def _create_cm3_special_tokens(self):
self.cm3_sentinel_end = "<eoss>"
self.cm3_break = "<racm3:break>"
self.dictionary.add_symbol(self.cm3_break)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all looking great.
We want to make a change here to recycle unused embedding index of cm3_break and sentinel for the next version.
Should i just add a commit on top of this PR? Or do I file a different PR?

@@ -200,24 +209,31 @@ def get_document_boundaries(self, item: torch.Tensor):
boundaries = boundaries + [item.size(0)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is get_document_boundaries() robust to the case that there is no break tokens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants