Skip to content

Conversation

@SherlockNoMad
Copy link
Contributor

As title, compile.enable should be False in the compiler toolkit style workflow

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 28, 2025
Copy link
Member

@xmfan xmfan left a comment

Choose a reason for hiding this comment

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

seems weird, if anything shouldn't we assert it to always be True?

@ruisizhang123
Copy link
Contributor

ruisizhang123 commented Oct 28, 2025

curious: currently, loss is compiled separately from model. This parallelize.py only controls model compilation part.

Do we want model's loss to be not compiled at all, or we still want users to control loss compilation?

Maybe it's better to check if "model" is in compile.components here? https://github.com/pytorch/torchtitan/blob/main/torchtitan/models/llama3/train_configs/debug_model.toml#L69

job_config: JobConfig,
) -> CompiledModule:
assert (
not job_config.compile.enable
Copy link
Contributor

@fegin fegin Oct 28, 2025

Choose a reason for hiding this comment

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

Suggested change
not job_config.compile.enable
"model" not in job_config.compile.components

@fegin
Copy link
Contributor

fegin commented Oct 28, 2025

Echo to @xmfan's comment. The compile flag must be set to false to enable the compiler toolkit, which seems counterintuitive.

Copy link
Contributor

@yiming0416 yiming0416 left a comment

Choose a reason for hiding this comment

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

Asserting compile.enable=False indeed seems counterintuitive under the compiler_toolkit experiment. WDYT about the following:
Refactor parallelize_deepseekv3() in experiments/simple_fsdp/deepseek_v3/parallelize.py into a parallelize_base_fn which contains everything until the line of if job_config.compile.enable

Then we import this parallelize_base_fn in the compiler_toolkit experiments folder. Then compile.enable=True, we apply the CompiledModule wrapper. In this way, we always assert compile.enable=True

@ruisizhang123
Copy link
Contributor

ehhh, I think it would be better to keep simplefsdp's parallelize function close to model/deepseek_v3/parallelize.py, which would be easier to integrate new features from main/deepseek_v3.

Why don't we add a context manager to temporary override job_config.compile to False when calling simple_fsdp_parallelize_llama.... I don't think it would be so counterintuitive as long as you leave a comment saying you are doing region compile later.

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

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants