Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve site package pathing #781

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

WangGithubUser
Copy link
Contributor

@WangGithubUser WangGithubUser commented Aug 21, 2023

Pre-submission checklist

  • I've ran the linters locally and fixed lint errors related to the files I modified in this PR. You can install the linters by running pip install -r requirements-dev.txt && pre-commit install
  • pre-commit run

Summary

A top-level package could implement by many ways, like .so, .pyd file, etc.
However, pyre only treat a .py file as a top-level package:

module_suffix = ".py" if self.is_toplevel_module else ""
return self.package_name + module_suffix

This PR improved the pathing by seeing the RECORD file in the package'sdist-info
e.g.This is the RECORD file in the dist-info of the ujson(ujson-5.8.0.dist-info):

ujson-5.8.0.dist-info/INSTALLER,sha256=zuuue4knoyJ-UwPPXg8fezS7VCrXJQrAP7zeNuwvFQg,4
ujson-5.8.0.dist-info/LICENSE.txt,sha256=NoZjRPCkhlQb2YAPGkB6BWwMIUqCxljwWWlNrz3DlNI,5975
ujson-5.8.0.dist-info/METADATA,sha256=QjK2Swt-E5vB5zyhRdsqHQ3iNODpRnbhQBXI-MPpTIM,8691
ujson-5.8.0.dist-info/RECORD,,
ujson-5.8.0.dist-info/REQUESTED,sha256=47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU,0
ujson-5.8.0.dist-info/WHEEL,sha256=iZaXX0Td62Nww8bojl0E84uJHjT41csHPKZmbUBbJPs,152
ujson-5.8.0.dist-info/top_level.txt,sha256=IBMYuVICHWi9xnmMN22UNMEFYf14TccC7o9uYPhycg0,6
ujson.cpython-310-x86_64-linux-gnu.so,sha256=Zk7sJGRXyPb_nJdf2ArotWK0zAFBWrcRwGJMfMXMHZ8,106000

As you see, We only need ujson.cpython-310-x86_64-linux-gnu.so but there are a lot of other unnessarry files in the RECORD.For this, I filterd them out by matching if the file is in XXX-XXX.dist-info or __pycache__.

Also see #773 (comment).

Issue fixed: #773

Test Plan

Before this PR:
image

After this PR:

$ ls -A
.pyre  .pyre_configuration  pyre-check

$ cat .pyre_configuration
{
  "site_package_search_strategy": "pep561",
  "source_directories": [
    "."
  ],
  "strict": true,
  "search_path": [
    {
      "site-package": "ujson",
      "is_toplevel_module": true
    }
  ],
  "exclude": [
    "pyre-check"
  ]
}

$ python -m pyre-check.client.pyre --noninteractive check # PR 781
2023-09-17 10:37:08,345 [PID 1496] INFO No binary specified, looking for `pyre.bin` in PATH
2023-09-17 10:37:08,347 [PID 1496] INFO Pyre binary is located at `/home/cbw/.local/bin/pyre.bin`
2023-09-17 10:37:08,351 [PID 1496] INFO Could not determine the number of Pyre workers from configuration. Auto-set the value to 7.
2023-09-17 10:37:08,355 [PID 1496] INFO No typeshed specified, looking for it...
2023-09-17 10:37:08,357 [PID 1496] INFO Found: `/usr/lib/pyre_check/typeshed`
2023-09-17 10:37:08,360 [PID 1496] INFO Writing arguments into /tmp/pyre_arguments_ym16vc1b.json...
2023-09-17 10:37:08,362 [PID 1496] DEBUG Arguments:
{
  "source_paths": {
    "kind": "simple",
    "paths": [
      "/mnt/d/Downloads/TestPyre"
    ]
  },
  "search_paths": [
    "/home/cbw/.local/lib/python3.10/site-packages$ujson.cpython-310-x86_64-linux-gnu.so",
    ...<I omited other search_paths>
  ],
  "excludes": [
    "pyre-check"
  ],
  "checked_directory_allowlist": [
    true
  ],
  "checked_directory_blocklist": [],
  "extensions": [],
  "log_path": "/mnt/d/Downloads/TestPyre/.pyre",
  "global_root": "/mnt/d/Downloads/TestPyre",
  "debug": false,
  "python_version": {
    "major": 3,
    "minor": 10,
    "micro": 12
  },
  "shared_memory": {},
  "parallel": true,
  "number_of_workers": 7,
  "additional_logging_sections": [
    "-progress"
  ],
  "show_error_traces": false,
  "strict": true
}
2023-09-17 10:37:08,407 [PID 1496] ERROR  Malformed server specification JSON. Expected string, got bool
2023-09-17 10:37:08,411 [PID 1496] ERROR Check command exited with non-zero return code: 1.

client/configuration/search_path.py Fixed Show fixed Hide fixed
client/configuration/search_path.py Fixed Show fixed Hide fixed
client/configuration/search_path.py Fixed Show fixed Hide fixed
client/configuration/search_path.py Fixed Show fixed Hide fixed
client/configuration/search_path.py Fixed Show resolved Hide resolved
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch 2 times, most recently from de998fd to a1ddd8f Compare August 21, 2023 05:40
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch 3 times, most recently from 25b3ba8 to eb713ca Compare August 21, 2023 06:01
@WangGithubUser WangGithubUser marked this pull request as draft August 22, 2023 01:59
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from 3515d36 to 66edd67 Compare August 22, 2023 03:48
@WangGithubUser WangGithubUser marked this pull request as ready for review August 22, 2023 05:28
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from bb2e017 to 660ac4c Compare August 22, 2023 09:36
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch 5 times, most recently from 512b3da to 93863db Compare August 22, 2023 09:59
@facebook-github-bot
Copy link
Contributor

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

@kinto0
Copy link
Contributor

kinto0 commented Sep 8, 2023

Hi! It seems like your issue is site packages are not being detected? Is this on windows? Would you be able to try on WSL since we currently do not support windows?

If the `excepted_path` is not None, it will must be a exists path
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from e03afd8 to 7e6afcb Compare September 9, 2023 04:36
@WangGithubUser
Copy link
Contributor Author

WangGithubUser commented Sep 9, 2023

Would you be able to try on WSL since we currently do not support windows?

image
(on WSL)

image

module_suffix = ".py" if self.is_toplevel_module else ""
return self.package_name + module_suffix

Pyre only treat a .py file as a site-package, and this PR improved the pathing of site-package.

@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from 1ae4897 to 98dd4f7 Compare September 9, 2023 06:01
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from 98dd4f7 to 1608b6a Compare September 9, 2023 06:05
@WangGithubUser WangGithubUser marked this pull request as draft September 10, 2023 04:03
client/backend_arguments.py Fixed Show fixed Hide fixed
client/backend_arguments.py Fixed Show fixed Hide fixed
@kinto0
Copy link
Contributor

kinto0 commented Sep 13, 2023

I'm not convinced yet that this is the right approach. I left some comments but I'll also bring it up with the team.

General feedback:

  • could you put more details in the summary?
  • could you do a before / after end to end test of this behavior? It's important to understand how this search_path changes the ocaml daemon functionality

client/backend_arguments.py Outdated Show resolved Hide resolved
client/backend_arguments.py Outdated Show resolved Hide resolved
client/configuration/search_path.py Outdated Show resolved Hide resolved
client/configuration/search_path.py Show resolved Hide resolved
@@ -35,11 +44,11 @@ def _expand_relative_root(path: str, relative_root: str) -> str:

class Element(abc.ABC):
@abc.abstractmethod
def path(self) -> str:
def path(self) -> Union[str, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Optional[str]

could you explain the reasoning and semantic differences for making this optional in the summary?

client/configuration/search_path.py Outdated Show resolved Hide resolved
client/configuration/search_path.py Show resolved Hide resolved
@WangGithubUser WangGithubUser force-pushed the Fix_site_package_pathing branch from dbb289a to 6bedb96 Compare September 15, 2023 10:30
@WangGithubUser
Copy link
Contributor Author

Here is a tasklist for this PR:

  1. Add a cache system for files in site_root or the check command will be very slow.
  2. Fix error Malformed server specification JSON. Expected string, got bool.
  3. Factor out the logic of path func of SitePackageElement.

@WangGithubUser
Copy link
Contributor Author

@kinto0 For the second task, I guess this may because a .so can't detect it's annotations.But I'm not sure, could you give me more info about this?

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

Successfully merging this pull request may close these issues.

Pyre couldn't find a required site-package
3 participants