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 python.DetectInterpreters and other utils #805

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Added python.DetectInterpreters and other utils #805

merged 1 commit into from
Oct 3, 2023

Conversation

nfx
Copy link
Collaborator

@nfx nfx commented Sep 26, 2023

This PR adds a few utilities related to Python interpreter detection:

  • python.DetectInterpreters to detect all Python versions available in $PATH by executing every matched binary name with --version flag.
  • python.DetectVirtualEnvPath to detect if there's any child virtual environment in src directory
  • python.DetectExecutable to detect if there's python3 installed either by which python3 command or by calling python.DetectInterpreters().AtLeast("v3.8")

To be merged after #804, as one of the steps to get #637 in, as previously discussed.

@nfx nfx requested review from pietern and andrewnester September 26, 2023 11:31
@nfx
Copy link
Collaborator Author

nfx commented Sep 26, 2023

Optionally, bca2bc2 could be reverted

@hamza-db
Copy link

one thing that can be nice to have.
is not not run any python binary that sits in a writable folder by all users.
this is mainly to avoid breaking the security model on multi-user system .. I know if the PATH is pointing somewhere untrusted it is the user fault. but we can help here

@hamza-db
Copy link

test test :
give a binary 777 and we shouldn't run it if it is 777 or inside a folder that is 777
but the main thing we check for for example in linux is : o=w

@nfx
Copy link
Collaborator Author

nfx commented Sep 26, 2023

Windows tests fail:

  • TestFilteringInterpreters
  • TestDetectFailsNoMinimalVersion
  • TestDetectsViaListing

need to re-test this from a windows VM or codespace.

@andrewnester
Copy link
Contributor

Overall, LGTM. What's unclear is why do we need this change exactly? Why do we need to know all Python interpreters available in the PATH?

@nfx
Copy link
Collaborator Author

nfx commented Sep 26, 2023

@andrewnester two cases:

  • a lot of dev environments have more than one Python interpreter (like our dev machines at Databricks) and frequently it's unclear/non-usable to get the right one consistently
  • in labs, we have projects that do require python3.10, and we want to fail the installation if a customer has only 3.7 or 3.6 installed.

libs/python/detect.go Show resolved Hide resolved
libs/python/detect.go Outdated Show resolved Hide resolved
libs/python/interpreters.go Outdated Show resolved Hide resolved
libs/python/interpreters.go Outdated Show resolved Hide resolved
libs/python/venv.go Show resolved Hide resolved
Base automatically changed from libs/process to main September 27, 2023 09:11
@nfx nfx force-pushed the libs/python branch 2 times, most recently from 8ed14e6 to 3063818 Compare September 29, 2023 16:51
@nfx
Copy link
Collaborator Author

nfx commented Oct 2, 2023

Will probably fall back to giving recommendation to install official python binaries instead of ms-windows-store...
image

@nfx
Copy link
Collaborator Author

nfx commented Oct 2, 2023

And Python interpreters from Microsoft Store are detected:
image

@nfx
Copy link
Collaborator Author

nfx commented Oct 2, 2023

users have to make sure that every installation is in PATH:
image

@nfx
Copy link
Collaborator Author

nfx commented Oct 2, 2023

and we do support a wild mix of different python interpreter versions from Microsoft Store and Official Python downloads:
image

@nfx
Copy link
Collaborator Author

nfx commented Oct 2, 2023

Stock Azure Windows VM is having all the important folders writeable as returned by os.Stat(dir).Mode().Perm():
image

see also: https://github.com/golang/go/blob/master/src/os/exec/lp_windows.go#L134-L206

@nfx nfx requested a review from pietern October 2, 2023 11:38
@hamza-db
Copy link

hamza-db commented Oct 2, 2023

I think the writability test was a "nice to have" measure but not a "must have".
so we can skip the checks if the Python setup is not secure by default on Windows.

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Nice work. This will make working with Python on Windows much, much easier.

libs/python/detect_unix_test.go Outdated Show resolved Hide resolved
libs/python/detect_win_test.go Outdated Show resolved Hide resolved
libs/python/interpreters.go Outdated Show resolved Hide resolved
libs/python/interpreters.go Outdated Show resolved Hide resolved
libs/python/interpreters_unix_test.go Outdated Show resolved Hide resolved
libs/python/interpreters_win_test.go Show resolved Hide resolved
@nfx nfx requested a review from pietern October 3, 2023 09:27
@nfx nfx added the Enhancement New feature or request label Oct 3, 2023
@nfx nfx enabled auto-merge October 3, 2023 10:38
This PR adds a few utilities related to Python interpreter detection:

- `python.DetectInterpreters` to detect all Python versions available in `$PATH` by executing every matched binary name with `--version` flag.
- `python.DetectVirtualEnvPath` to detect if there's any child virtual environment in `src` directory
- `python.DetectExecutable` to detect if there's python3 installed either by `which python3` command or by calling `python.DetectInterpreters().AtLeast("v3.8")`

Code coverage is 95%

Remove unused code

some comments

don't execute from world-writeable locations

more tests

make things work on windows

shuffle some code around
@nfx nfx changed the title Added python.DetectInterpreters other utils Added python.DetectInterpreters and other utils Oct 3, 2023
@nfx nfx added this pull request to the merge queue Oct 3, 2023
Merged via the queue into main with commit 7d0f170 Oct 3, 2023
@nfx nfx deleted the libs/python branch October 3, 2023 10:53
@andrewnester
Copy link
Contributor

Fixes #818

andrewnester added a commit that referenced this pull request Oct 4, 2023
CLI:
 * Refactor change computation for sync ([#785](#785)).

Bundles:
 * Allow digits in the generated short name ([#820](#820)).
 * Emit an error when incompatible all purpose cluster used with Python wheel tasks ([#823](#823)).
 * Use normalized short name for tag value in development mode ([#821](#821)).
 * Added `python.DetectInterpreters` and other utils ([#805](#805)).
 * Mark artifacts properties as optional ([#834](#834)).
 * Added support for glob patterns in pipeline libraries section ([#833](#833)).

Internal:
 * Run tests to verify backend tag validation behavior ([#814](#814)).
 * Library to validate and normalize cloud specific tags ([#819](#819)).
 * Added test to submit and run various Python tasks on multiple DBR versions ([#806](#806)).
 * Create a release PR in setup-cli repo on tag push ([#827](#827)).

API Changes:
 * Changed `databricks account metastore-assignments list` command to return .
 * Changed `databricks jobs cancel-all-runs` command with new required argument order.
 * Added `databricks account o-auth-published-apps` command group.
 * Changed `databricks serving-endpoints query` command . New request type is .
 * Added `databricks serving-endpoints patch` command.
 * Added `databricks credentials-manager` command group.
 * Added `databricks settings` command group.
 * Changed `databricks clean-rooms list` command to require request of .
 * Changed `databricks statement-execution execute-statement` command with new required argument order.

OpenAPI commit bcbf6e851e3d82fd910940910dd31c10c059746c (2023-10-02)
Dependency updates:
 * Bump github.com/google/uuid from 1.3.0 to 1.3.1 ([#825](#825)).
 * Updated Go SDK to 0.22.0 ([#831](#831)).
github-merge-queue bot pushed a commit that referenced this pull request Oct 4, 2023
CLI:
* Refactor change computation for sync
([#785](#785)).

Bundles:
* Allow digits in the generated short name
([#820](#820)).
* Emit an error when incompatible all purpose cluster used with Python
wheel tasks ([#823](#823)).
* Use normalized short name for tag value in development mode
([#821](#821)).
* Added `python.DetectInterpreters` and other utils
([#805](#805)).
* Mark artifacts properties as optional
([#834](#834)).
* Added support for glob patterns in pipeline libraries section
([#833](#833)).

Internal:
* Run tests to verify backend tag validation behavior
([#814](#814)).
* Library to validate and normalize cloud specific tags
([#819](#819)).
* Added test to submit and run various Python tasks on multiple DBR
versions ([#806](#806)).
* Create a release PR in setup-cli repo on tag push
([#827](#827)).

API Changes:
* Changed `databricks account metastore-assignments list` command to
return .
* Changed `databricks jobs cancel-all-runs` command with new required
argument order.
 * Added `databricks account o-auth-published-apps` command group.
* Changed `databricks serving-endpoints query` command . New request
type is .
 * Added `databricks serving-endpoints patch` command.
 * Added `databricks credentials-manager` command group.
 * Added `databricks settings` command group.
 * Changed `databricks clean-rooms list` command to require request of .
* Changed `databricks statement-execution execute-statement` command
with new required argument order.

OpenAPI commit bcbf6e851e3d82fd910940910dd31c10c059746c (2023-10-02)
Dependency updates:
* Bump github.com/google/uuid from 1.3.0 to 1.3.1
([#825](#825)).
* Updated Go SDK to 0.22.0
([#831](#831)).
hectorcast-db pushed a commit that referenced this pull request Oct 13, 2023
This PR adds a few utilities related to Python interpreter detection:

- `python.DetectInterpreters` to detect all Python versions available in
`$PATH` by executing every matched binary name with `--version` flag.
- `python.DetectVirtualEnvPath` to detect if there's any child virtual
environment in `src` directory
- `python.DetectExecutable` to detect if there's python3 installed
either by `which python3` command or by calling
`python.DetectInterpreters().AtLeast("v3.8")`

To be merged after #804, as one of
the steps to get #637 in, as
previously discussed.
hectorcast-db pushed a commit that referenced this pull request Oct 13, 2023
CLI:
* Refactor change computation for sync
([#785](#785)).

Bundles:
* Allow digits in the generated short name
([#820](#820)).
* Emit an error when incompatible all purpose cluster used with Python
wheel tasks ([#823](#823)).
* Use normalized short name for tag value in development mode
([#821](#821)).
* Added `python.DetectInterpreters` and other utils
([#805](#805)).
* Mark artifacts properties as optional
([#834](#834)).
* Added support for glob patterns in pipeline libraries section
([#833](#833)).

Internal:
* Run tests to verify backend tag validation behavior
([#814](#814)).
* Library to validate and normalize cloud specific tags
([#819](#819)).
* Added test to submit and run various Python tasks on multiple DBR
versions ([#806](#806)).
* Create a release PR in setup-cli repo on tag push
([#827](#827)).

API Changes:
* Changed `databricks account metastore-assignments list` command to
return .
* Changed `databricks jobs cancel-all-runs` command with new required
argument order.
 * Added `databricks account o-auth-published-apps` command group.
* Changed `databricks serving-endpoints query` command . New request
type is .
 * Added `databricks serving-endpoints patch` command.
 * Added `databricks credentials-manager` command group.
 * Added `databricks settings` command group.
 * Changed `databricks clean-rooms list` command to require request of .
* Changed `databricks statement-execution execute-statement` command
with new required argument order.

OpenAPI commit bcbf6e851e3d82fd910940910dd31c10c059746c (2023-10-02)
Dependency updates:
* Bump github.com/google/uuid from 1.3.0 to 1.3.1
([#825](#825)).
* Updated Go SDK to 0.22.0
([#831](#831)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants