Skip to content

Conversation

kiukchung
Copy link
Contributor

Summary:
Fixes the following two scenarios:

#1

~/bin
    - python
    - penv.par

$ cd ~/bin
$ ./python -m mo.du.le

Does not pick up ~/bin/penv.par (as expected) because sys.argv[0] = ./python. Fix is to return (Path(sys.argv[0])/"penv.par").absolute()

#2
Using this interpreter as a unittest resource + torchx local_cwd scheduler does not resolve penv.par from the test source dir since the symlink is resolved by unix and sys.argv[0] is set to the actual interpreter (not the symlinked one)

python_unittest(
   name="foo_test",
   resource={
       "//torchx/fb:python": "python",
       "//torchx/apps:penv": "penv.par",
   }
   ...
)

# buck run mode/opt //foo_test
# outputs in build dir

~/fbcode/buck-out/opt/gen/foo_test#link-tree/
    - python -> buck-out/opt/gen/torchx/fb/python.par
    - penv.par

Hence with the current penv par lookup logic we end up looking for a penv.par in buck-out/opt/gen/torchx/fb/penv.par which does not exist and we just simply fall back to system python.

Reviewed By: aivanou

Differential Revision: D31402466

…inding penv.par, add a CWD/penv.par search fallback to handle unittest @mode/opt

Summary:
Fixes the following two scenarios:

#1
```
~/bin
    - python
    - penv.par

$ cd ~/bin
$ ./python -m mo.du.le
```
Does not pick up `~/bin/penv.par` (as expected) because `sys.argv[0] = ./python`. Fix is to return `(Path(sys.argv[0])/"penv.par").absolute() `

#2
Using this interpreter as a unittest resource + torchx local_cwd scheduler does not resolve penv.par from the test source dir since the symlink is resolved by unix and `sys.argv[0]` is set to the actual interpreter (not the symlinked one)
```
python_unittest(
   name="foo_test",
   resource={
       "//torchx/fb:python": "python",
       "//torchx/apps:penv": "penv.par",
   }
   ...
)

# buck run mode/opt //foo_test
# outputs in build dir

~/fbcode/buck-out/opt/gen/foo_test#link-tree/
    - python -> buck-out/opt/gen/torchx/fb/python.par
    - penv.par
```
Hence with the current penv par lookup logic we end up looking for a penv.par in `buck-out/opt/gen/torchx/fb/penv.par` which does not exist and we just simply fall back to system python.

Reviewed By: aivanou

Differential Revision: D31402466

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

This pull request was exported from Phabricator. Differential Revision: D31402466

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 5, 2021
@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #216 (9c0b722) into main (cf95835) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #216   +/-   ##
=======================================
  Coverage   91.10%   91.10%           
=======================================
  Files          57       57           
  Lines        2630     2630           
=======================================
  Hits         2396     2396           
  Misses        234      234           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf95835...9c0b722. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants