Skip to content

Conversation

XuehaiPan
Copy link
Collaborator

@XuehaiPan XuehaiPan commented Feb 21, 2023

Changes:

  1. Recognize .py.in and .pyi.in files as Python in VS Code for a better development experience.
  2. Fix deep setting merge in tools/vscode_settings.py.
  1. Use Namedtuple rather than namedtuple + __annotations__ for torch.nn.utils.rnn.PackedSequence_:

    namedtuple + __annotations__:

    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])
    
    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}

    Namedtuple: Python 3.6+

    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
  1. Sort import statements and remove unnecessary imports in .pyi, .pyi.in files.
  2. Format .pyi, .pyi.in files and remove unnecessary ellipsis ... in type stubs.

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 21, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 391b2f5:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: distributed (c10d) release notes category label Feb 21, 2023
@XuehaiPan XuehaiPan force-pushed the vscode-py-in branch 2 times, most recently from 1e997b5 to f10b578 Compare February 21, 2023 13:11
@XuehaiPan XuehaiPan changed the title Recognize .py.in and .pyi.in files as Python in VS Code Update .pyi Python stub files and recognize .py.in and .pyi.in files as Python in VS Code Feb 21, 2023
Copy link
Collaborator Author

@XuehaiPan XuehaiPan Feb 21, 2023

Choose a reason for hiding this comment

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

We should not use mutable variables as function defaults.

Ref: dangerous-default-value / W0102

Correct code:

def func(
    ...,
    device_maps: Optional[Dict[str, Dict[torch.device, torch.device]]] = None,
    devices: Optional[List[torch.device]] = None
):
    if device_maps is None:
        device_maps = {}
    if devices is None:
        devices = []
    ...

@XuehaiPan XuehaiPan force-pushed the vscode-py-in branch 4 times, most recently from fa34621 to 5013d55 Compare February 21, 2023 14:24
@albanD
Copy link
Collaborator

albanD commented Feb 21, 2023

Thanks for the PR!
Could you split this up into formatting-only PRs and the rest?
In particular, the PackedSequence_ change would need to be looked at carefully.

Also I'm not sure what the vscode_settings.py file does, how to test these changes?

@XuehaiPan XuehaiPan changed the title Update .pyi Python stub files and recognize .py.in and .pyi.in files as Python in VS Code [1/3] Update .pyi Python stub files and recognize .py.in and .pyi.in files as Python in VS Code Feb 22, 2023
@XuehaiPan
Copy link
Collaborator Author

XuehaiPan commented Feb 22, 2023

Could you split this up into formatting-only PRs and the rest?
In particular, the PackedSequence_ change would need to be looked at carefully.

I split this into 3 PRs. The PackedSequence_ change is moved to a standalone PR.

Also I'm not sure what the vscode_settings.py file does, how to test these changes?

You could run:

./tools/vscode_settings.py

or

python3 ./tools/vscode_settings.py

It will update the workspace VS Code settings .vscode/settings.json from .vscode/settings_recommended.json. The script will create a setting file if not exist.

For example:

current file (.vscode/settings.json):

{
    "[c]": {
        "editor.insertSpaces": true,
        "editor.tabSize": 2
    },
    "[cpp]": {
        "editor.insertSpaces": true,
        "editor.tabSize": 2,
        "editor.defaultFormatter": "ms-vscode.cpptools"
    },
    "[python]": {
        "editor.defaultFormatter": "ms-python.black-formatter"
    },
    "files.associations": {
        "aten/src/ATen/ATenConfig.cmake.in": "cmake"
    },
    "java.autobuild.enabled": false,
    "java.compile.nullAnalysis.mode": "disabled",
}

after running ./tools/vscode_settings.py:

{
    "[c]": {
        "editor.insertSpaces": true,
        "editor.tabSize": 2,
    },
    "[cpp]": {
        "editor.insertSpaces": true,
        "editor.tabSize": 2,
        "editor.defaultFormatter": "ms-vscode.cpptools",
    },
    "[python]": {
        "editor.defaultFormatter": "ms-python.black-formatter",
+       "editor.tabSize": 4,
    },
    "files.associations": {
        "aten/src/ATen/ATenConfig.cmake.in": "cmake",
+       "*.py.in": "python",
+       "*.pyi.in": "python",
    },
    "java.autobuild.enabled": false,
    "java.compile.nullAnalysis.mode": "disabled",
+   "files.eol": "\n",
+   "files.insertFinalNewline": true,
+   "files.trimFinalNewlines": true,
+   "files.trimTrailingWhitespace": true,
+   "python.formatting.provider": "none",
+   "python.linting.enabled": true,
+   "python.linting.flake8Enabled": true,
}

@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Feb 22, 2023
Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

Thanks for the change! Looks fine overall, just had some minor points :D

current_settings = json.loads(current_settings_text)
except ValueError: # json.JSONDecodeError is a subclass of ValueError
if HAS_JSON5:
raise SystemExit("Failed to parse .vscode/settings.json.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why SystemExit and not RuntimeError?

Copy link
Collaborator Author

@XuehaiPan XuehaiPan Mar 1, 2023

Choose a reason for hiding this comment

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

Since tools/vscode_settings.py is a script to run in the shell. It is not meant to be imported, so its functions will not be put in try-execpt blocks in other scripts.

Raising SystemExit is equivalent to printing an error message and exiting with code 1:

$ pip3 uninstall json5
$ ./tools/vscode_settings.py
Failed to parse .vscode/settings.json. Maybe it contains comments or trailing commas. Try `pip install json5` to install an extended JSON parser.
$ echo $?
1

Using RuntimeError:

$ pip3 uninstall json5
$ ./tools/vscode_settings.py
Traceback (most recent call last):
  File "/home/PanXuehai/Projects/pytorch/./tools/vscode_settings.py", line 42, in main
    current_settings = json.loads(current_settings_text)
  File "/home/PanXuehai/Miniconda3/envs/pytorch-dev/lib/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/home/PanXuehai/Miniconda3/envs/pytorch-dev/lib/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/home/PanXuehai/Miniconda3/envs/pytorch-dev/lib/python3.10/json/decoder.py", line 353, in raw_decode
    obj, end = self.scan_once(s, idx)
json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 5 column 5 (char 85)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/PanXuehai/Projects/pytorch/./tools/vscode_settings.py", line 65, in <module>
    main()
  File "/home/PanXuehai/Projects/pytorch/./tools/vscode_settings.py", line 46, in main
    raise RuntimeError(
RuntimeError: Failed to parse .vscode/settings.json. Maybe it contains comments or trailing commas. Try `pip install json5` to install an extended JSON parser.
$ echo $?
1

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thank you for explaining!

@XuehaiPan
Copy link
Collaborator Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 1, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@XuehaiPan XuehaiPan changed the title [1/3] Update .pyi Python stub files and recognize .py.in and .pyi.in files as Python in VS Code [1/3] Recognize .py.in and .pyi.in files as Python in VS Code Mar 1, 2023
@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Mar 1, 2023
pytorchmergebot pushed a commit that referenced this pull request Mar 1, 2023
…e annotated `NamedTuple` (#95267)

Changes:

- #95200

1. Recognize `.py.in` and `.pyi.in` files as Python in VS Code for a better development experience.
2. Fix deep setting merge in `tools/vscode_settings.py`.

- => this PR: #95267

3. Use `Namedtuple` rather than `namedtuple + __annotations__` for `torch.nn.utils.rnn.PackedSequence_`:

    `namedtuple + __annotations__`:

    ```python
    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])

    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}
    ```

    `Namedtuple`: Python 3.6+

    ```python
    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
    ```

- #95268

4. Sort import statements and remove unnecessary imports in `.pyi`, `.pyi.in` files.
5. Format `.pyi`, `.pyi.in` files and remove unnecessary ellipsis `...` in type stubs.
Pull Request resolved: #95267
Approved by: https://github.com/janeyx99
pytorchmergebot pushed a commit that referenced this pull request Mar 1, 2023
)

Changes:

- #95200

1. Recognize `.py.in` and `.pyi.in` files as Python in VS Code for a better development experience.
2. Fix deep setting merge in `tools/vscode_settings.py`.

- #95267

3. Use `Namedtuple` rather than `namedtuple + __annotations__` for `torch.nn.utils.rnn.PackedSequence_`:

    `namedtuple + __annotations__`:

    ```python
    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])

    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}
    ```

    `Namedtuple`: Python 3.6+

    ```python
    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
    ```

- => this PR: #95268

4. Sort import statements and remove unnecessary imports in `.pyi`, `.pyi.in` files.
5. Format `.pyi`, `.pyi.in` files and remove unnecessary ellipsis `...` in type stubs.
Pull Request resolved: #95268
Approved by: https://github.com/huydhn
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 2, 2023
…5200)

Changes:

- => this PR: #95200

1. Recognize `.py.in` and `.pyi.in` files as Python in VS Code for a better development experience.
2. Fix deep setting merge in `tools/vscode_settings.py`.

- #95267

3. Use `Namedtuple` rather than `namedtuple + __annotations__` for `torch.nn.utils.rnn.PackedSequence_`:

    `namedtuple + __annotations__`:

    ```python
    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])

    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}
    ```

    `Namedtuple`: Python 3.6+

    ```python
    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
    ```

- #95268

4. Sort import statements and remove unnecessary imports in `.pyi`, `.pyi.in` files.
5. Format `.pyi`, `.pyi.in` files and remove unnecessary ellipsis `...` in type stubs.
Pull Request resolved: pytorch/pytorch#95200
Approved by: https://github.com/janeyx99
@XuehaiPan XuehaiPan deleted the vscode-py-in branch March 2, 2023 05:49
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 5, 2023
…5200)

Changes:

- => this PR: #95200

1. Recognize `.py.in` and `.pyi.in` files as Python in VS Code for a better development experience.
2. Fix deep setting merge in `tools/vscode_settings.py`.

- #95267

3. Use `Namedtuple` rather than `namedtuple + __annotations__` for `torch.nn.utils.rnn.PackedSequence_`:

    `namedtuple + __annotations__`:

    ```python
    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])

    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}
    ```

    `Namedtuple`: Python 3.6+

    ```python
    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
    ```

- #95268

4. Sort import statements and remove unnecessary imports in `.pyi`, `.pyi.in` files.
5. Format `.pyi`, `.pyi.in` files and remove unnecessary ellipsis `...` in type stubs.
Pull Request resolved: pytorch/pytorch#95200
Approved by: https://github.com/janeyx99
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 5, 2023
…5200)

Changes:

- => this PR: #95200

1. Recognize `.py.in` and `.pyi.in` files as Python in VS Code for a better development experience.
2. Fix deep setting merge in `tools/vscode_settings.py`.

- #95267

3. Use `Namedtuple` rather than `namedtuple + __annotations__` for `torch.nn.utils.rnn.PackedSequence_`:

    `namedtuple + __annotations__`:

    ```python
    PackedSequence_ = namedtuple('PackedSequence_',
                                 ['data', 'batch_sizes', 'sorted_indices', 'unsorted_indices'])

    # type annotation for PackedSequence_ to make it compatible with TorchScript
    PackedSequence_.__annotations__ = {'data': torch.Tensor, 'batch_sizes': torch.Tensor,
                                       'sorted_indices': Optional[torch.Tensor],
                                       'unsorted_indices': Optional[torch.Tensor]}
    ```

    `Namedtuple`: Python 3.6+

    ```python
    class PackedSequence_(NamedTuple):
        data: torch.Tensor
        batch_sizes: torch.Tensor
        sorted_indices: Optional[torch.Tensor]
        unsorted_indices: Optional[torch.Tensor]
    ```

- #95268

4. Sort import statements and remove unnecessary imports in `.pyi`, `.pyi.in` files.
5. Format `.pyi`, `.pyi.in` files and remove unnecessary ellipsis `...` in type stubs.
Pull Request resolved: pytorch/pytorch#95200
Approved by: https://github.com/janeyx99
pruthvistony added a commit to ROCm/pytorch that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: distributed (c10d) release notes category topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants