[ExecuTorch] Separate extension.Module Namespaces from Aten and non-Aten

[zhenyan-zhang-meta]

Stack from ghstack (oldest at bottom):

• [ExecuTorch][#10447] Extend PyBundledModule with extension.BundledModule #10450
• [ExecuTorch][#10375] Add extension.BundledModule to Wrap extension.Module with Bundled Program Logic #10449
• -> [ExecuTorch] Separate extension.Module Namespaces from Aten and non-Aten #10576

Context

Separate extension.Module Namespaces to be executorch::extension::module and executorch::extension::module::aten, otherwise if a package relies on both aten and non-aten of the same implementation and the namespace is the same, there will be duplicate symbol issue like:

ld.lld: error: duplicate symbol: vtable for executorch::extension::Module
>>> defined at {redacted}/executorch/extension/module/__module__/__stripped__/module.cpp.pic.stripped.o:(vtable for executorch::extension::Module)
>>> defined at {redacted}/executorch/extension/module/__module_aten__/__stripped__/module.cpp.pic.stripped.o:

Proposal

Doing something similar to what we already did for bundled_program in #10307

Since extension.Module is a public API, we introduce a namespace alias, so that existing use cases won't get affected. Since namespace alias doesn't create additional symbols, there won't be duplicate symbol issue.

Differential Revision: D73903870

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10576

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D73903870

[shoumikhin]

Something seems off, not sure we can just introduce macros like that in public APIs.
Need more details on how we got here and potentially amend the precedent in #10307 too.

[zhenyan-zhang-meta]

@shoumikhin This is a fix to unblock future issues where we see targets using both aten and non-aten together and there will be issues like:

ld.lld: error: duplicate symbol: vtable for executorch::extension::Module
>>> defined at {redacted}/executorch/extension/module/__module__/__stripped__/module.cpp.pic.stripped.o:(vtable for executorch::extension::Module)
>>> defined at {redacted}/executorch/extension/module/__module_aten__/__stripped__/module.cpp.pic.stripped.o:

But the problem is that, in original implementation 361b295 it will break existing usages of the public API.

The workaround here is that we introduce a namespace alias, so that existing use cases won't get affected. Since namespace alias doesn't create additional symbols, there won't be duplicate symbol issue. The change now looks more straightforward.

Also cc @larryliu0820 @tarun292 if you have other context that could justify separating namespaces. At least I think since bundled_module is not a public API we might be fine for #10307.

Also cc @Gasoonjia

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D73903870

[facebook-github-bot]

This pull request was exported from Phabricator. Differential Revision: D73903870

[shoumikhin]

Sorry, I still don't get the bigger picture and need to look closer at where we're heading by introducing this pattern.
It looks like a workaround and I wonder if there are any alternatives to tackle this?
Also, @swolchok any thoughts?

[zhenyan-zhang-meta]

@shoumikhin I've provided some detailed context in the internal diff, basically we see places where both aten and non-aten version of pybindings is a dependent, and when we introduce extension.Module to pybindings, the aten and non-aten version of extension.Module will both be a dependent and there will be a conflict due to "duplicate symbol".

To justify the current change:

• It is backward compatible.
• It is harmless since at the moment there's no aten-specific code in this class.
• It brings benefit to separate aten and non-aten code to avoid duplicate symbol in the future.

[swolchok]

I'm skeptical of the original goal to support mixing ATen and non-ATen mode builds of ExecuTorch files in the same binary. I suspect that that is just asking for ODR violations to crop up eventually.

[zhenyan-zhang-meta]

@swolchok

I suspect that that is just asking for ODR violations to crop up eventually.

Is this a yes to this change, since this change fixes the ODR violation of this module.

Seems like similar things has been done for all deps of pybindings that has an aten and non-aten module, I found one more: #9894

cc @larryliu0820 @iseeyuan

[swolchok]

Is this a yes to this change, since this change fixes the ODR violation of this module.

No, I'm saying that we should instead stop mixing aten mode with non-aten mode.

[zhenyan-zhang-meta]

@swolchok What we could do if the module dependent on both aten and non-aten is not owned by us? Should we discard this change, roll back #9894 and #10307, and let the module owner fix their part on failed tests?

Provided more context for internal use cases in internal diff.