Skip to content

Conversation

davidberard98
Copy link
Contributor

@davidberard98 davidberard98 commented May 10, 2024

Stack from ghstack (oldest at bottom):

Motivating Issue:

We had this example:

# a.py
import torch
from math import sqrt

# This file is used by test_class_type_closure

class MySqrtClass:
    value: float

    def __init__(self):
        self.value = 4.0

    def get_sqrt_inverse(self) -> float:
        return sqrt(1.0 / self.value)
# b.py
import torch
from a import MySqrtClass

torch.jit.script(MySqrtClass)

This would fail to script:

Traceback (most recent call last):
  File "/data/users/dberard/scripts/ts_use_sqrt.py", line 10, in <module>
    torch.jit.script(ts_sqrt.MyConfig)
  File "/data/users/dberard/pytorch/torch/jit/_script.py", line 1378, in script
    _compile_and_register_class(obj, _rcb, qualified_name)
  File "/data/users/dberard/pytorch/torch/jit/_recursive.py", line 73, in _compile_and_register_class
    script_class = torch._C._jit_script_class_compile(
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError:
undefined value sqrt:
  File "/data/users/dberard/scripts/ts_sqrt.py", line 17
    def get_sqrt_inverse(self) -> float:
        return sqrt(1 / self.value)

Fix details:
In TorchScript, we use "resolution callbacks" - python callables that will map a string to an object. In this case, the resolution callback was unable to identify that sqrt was actually math.sqrt.

We have multiple types of resolution callbacks: two are createResolutionCallbackFromFrame and createResolutionCallbackFromClosure.

In this case, we were using createResolutionCallbackFromFrame. But this doesn't work if jit.script is called from a different file than the class definition.

Fix: createResolutionCallbackFromClosure for each of the methods on a class (which is similar to what we do for an nn.Module). Pass this into the C++ function so that we can use the individual RCBs for each of the methods. If a method doesn't have an RCB for some reason, then fallback to the old RCB method where we look at the current frame.

Differential Revision: D57224315

@pytorch-bot pytorch-bot bot added the release notes: jit release notes category label May 10, 2024
Copy link

pytorch-bot bot commented May 10, 2024

🔗 Helpful Links

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

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

❌ 13 New Failures

As of commit 2bd7446 with merge base 946b96f (image):

NEW FAILURES - The following jobs have failed:

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

davidberard98 added a commit that referenced this pull request May 10, 2024
@davidberard98 davidberard98 changed the title [JIT] Better support for non-nn.Module classes defined in different file [TorchScript] Better support for non-nn.Module classes defined in different file May 10, 2024
@davidberard98
Copy link
Contributor Author

@davidberard98 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Jul 9, 2024
@github-actions github-actions bot closed this Aug 8, 2024
@github-actions github-actions bot deleted the gh/davidberard98/297/head branch September 8, 2024 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: jit release notes category Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant