Do not use deprecated SourceFileLoader.load_module() in dynamic module loading

[XuehaiPan]

What does this PR do?

Remove deprecated SourceFileLoader.load_module() in dynamic module loading. Replace it with exec_module().

https://github.com/python/cpython/blob/e3671ead9419c7afab6a4e501ade86bc9bc10950/Lib/importlib/_bootstrap_external.py#L1166-L1176

    @_check_name
    def load_module(self, fullname):
        """Load a module from a file.


        This method is deprecated.  Use exec_module() instead.


        """
        # The only reason for this method is for the name check.
        # Issue #14857: Avoid the zero-argument form of super so the implementation
        # of that form can be updated without breaking the frozen module.
        return super(FileLoader, self).load_module(fullname)

load_module(fullname)

Deprecated since version 3.4: The recommended API for loading a module is exec_module() (and create_module()). Loaders should implement it instead of load_module(). The import machinery takes care of all the other responsibilities of load_module() when exec_module() is implemented.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[amyeroberts]

cc @ydshieh

[ydshieh]

Hi @XuehaiPan Thank you for opening this PR ❤️ .

Overall LGTM, but there is one test failing

tests/models/auto/test_modeling_auto.py::AutoModelTest::test_from_pretrained_dynamic_model_with_period

Could you check if it pass on main and if so why it starts to fail on this PR?

[XuehaiPan]

Hi @ydshieh, I have fixed the test error.

[ydshieh]

Thanks! CI is green. I will check !

[ydshieh]

Thank you for the iteration!

[ydshieh]

Ah, thank you for the quick action ❤️ . I was thinking probably better not to bother you further

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[tmm1]

@XuehaiPan why do you call exec_module in both cases here?

it has the effect of reloading code definitions from disk for every model instantiation, which breaks monkey-patches applied onto trust_remote_code models (i.e. liger-kernel, etc)

[ydshieh]

Hmm, I think here we did this to match the previous code block, where

module = importlib.machinery.SourceFileLoader(name, module_path).load_module()

was always executed.

[XuehaiPan]

@XuehaiPan why do you call exec_module in both cases here?

This intentional. The module instance will be inserted into sys.module during initialization, whether it is executed or not (partially initializaed). Maybe we can add a module.__initialized indicator to ensure the module file is executed only once. I will submit a PR to resolve this.

[XuehaiPan]

Addressed in #33162

• Load dynamic module (remote code) only once if code isn't change #33162