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

added support for Pysa Model Validation in VSCode Extension #433

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

m0mosenpai
Copy link
Contributor

Follow up to #409

This PR adds the implementation of model validation feature into Pysa's VSCode Plugin.

Working Examples

Screenshots showing the running extensions after these changes (the errors displayed are such due to misconfigured typeshed)

image

The edge case where Pyre is not able to parse the file, the following error is displayed and the extension continues to work:
image

Copy link
Contributor

@gbleaney gbleaney left a comment

Choose a reason for hiding this comment

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

Changes looking pretty good! Can you please rebase on master, now that we've landed your previous PR?

client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@m0mosenpai m0mosenpai requested a review from gbleaney July 19, 2021 08:37
Copy link
Contributor

@gbleaney gbleaney left a comment

Choose a reason for hiding this comment

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

Looks pretty great! Some really small comments. At the same time I'll import this and make sure it passes our internal CI

client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

client/commands/v2/pysa_server.py Outdated Show resolved Hide resolved
@gbleaney
Copy link
Contributor

@m0mosenpai there are two tests failing:
https://github.com/facebook/pyre-check/pull/433/checks?check_run_id=3111924028
https://github.com/facebook/pyre-check/pull/433/checks?check_run_id=3111924020

I believe you should be able to tweak both of the tests to do the import from one level higher (like you're doing locally). That should unblock landing this PR

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

2 similar comments
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@m0mosenpai
Copy link
Contributor Author

@m0mosenpai there are two tests failing:
https://github.com/facebook/pyre-check/pull/433/checks?check_run_id=3111924028
https://github.com/facebook/pyre-check/pull/433/checks?check_run_id=3111924020

I believe you should be able to tweak both of the tests to do the import from one level higher (like you're doing locally). That should unblock landing this PR

@gbleaney
I think i'm confused how to fix the tests. I see that pyre_test.py and the imports in __init__.py in tests folder are failing. I was under the assumption that the tests would fail since the top-level hack i did was only done locally (running pyre as python -m pyre-check.client.pyre ). I'm not sure how to replicate the behavior for the tests.

@gbleaney
Copy link
Contributor

gbleaney commented Aug 11, 2021

@m0mosenpai This is the command that the first test is running:
https://github.com/facebook/pyre-check/runs/3289612419?check_suite_focus=true#step:5:1

If you look into the script that runs that test, you can see that it just cds into the root of the repo, and runs the tests in the client folder:

cd "${SCRIPTS_DIRECTORY}/.."
echo ' Enumerating test files:'
files=$(find client -name '*_test.py' ! -name 'watchman_test.py')

You should be able to just tweak that test to cd up one directory higher, and run the tests in pyre-check/client rather than just client

This is the command that the second test is running:
https://github.com/facebook/pyre-check/runs/3289612418?check_suite_focus=true#step:8:4

You can see that is just runs a second script:

python3 ../../tools/pysa_integration_tests/run.py \
--skip-model-verification \
--run-from-source

That test invokes the Pyre CLI from inside the pyre-check folder:

command = [
"python",
"-m" "client.pyre",
"--noninteractive",
"analyze",
]

command, text=True, cwd=current_directory

You'll just need to change that to invoke from one dir higher, just like you did locally

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2021
Summary:
Related to discussion on this with gbleaney.

PR #433 use of relative imports causes `Import beyond top-level package` errors. Internally, relative imports are required to pass the tests whereas on GitHub, this causes issues. This PR fixes that issue by running tests from one directory higher, unblocking #433.

Pull Request resolved: #461

Reviewed By: dkgi

Differential Revision: D30518016

Pulled By: gbleaney

fbshipit-source-id: e7d76f2059bde5110849923887f928459228e0a8
@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@m0mosenpai has updated the pull request. You must reimport the pull request before landing.

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.

None yet

5 participants