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

Separate Python interpreter from inspector and pass selected python environment down through deployment #2556

Merged
merged 17 commits into from
Jan 28, 2025

Conversation

sagerb
Copy link
Collaborator

@sagerb sagerb commented Jan 25, 2025

Intent

This PR includes the functionality required to query and pass on the selected Python interpreter path active within VSCode or Positron and passes that down through the applicable APIs for reference within the agent. The agent then uses the selected Python interpreter when needed.

Resolves #2494
Resolves #2496

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

To accomplish having the availability of the Python active interpreter within the agent, I followed the pattern established when we refactored the R interpreter-related functionality.

This involved a number of incremental steps, which have been grouped into related commits:

User Impact

The publisher now recognizes the active python interpreter as selected within Microsoft's Python extension and Posit's Positron IDE. This leads to a more consistent and controllable environment for the users.

Automated Tests

Unit tests have been updated to reflect the changes.

Directions for Reviewers

I have grouped types of changes into individual commits. Reviewing them in that manner should help facilitate a more productive review of the many changes.

The bulk of the functionality change involves creating the new Python interpreter object, extracting this Python interpreter functionality out of the inspection package, and then assessing the impact that those changes had on the other code (especially on the initialize package functionality). In addition, some changes were then needed to extract the correct values from the VSCode/Positron environment and pass them through the API to be available within the API handlers.

The changes involved in this PR are best verified by inspection of our different content types / sample projects. There should be no difference in the detection of Python or not, but the version string will now be consistent with the active version of Python that the user is running (assuming that they are setting this through one of the facilities we support - VSCode Python extension, Positron Python selection or first location found searching the path).

Checklist

@sagerb sagerb self-assigned this Jan 25, 2025
@dotNomad
Copy link
Collaborator

Did some double checking due to the failing bats tests:

  • fastapi-simple deployed with no problems on Python 3.12
  • quarto-website-py had some Python typing errors so I updated the requirements.txt using a fresh venv and installing Jupyter. With that it deployed with no problems

It looks like the deploy.bats test creates requirements.txt files using the CLI, if I'm reading that correctly, so perhaps this change caused that to fail. It is only on the Python tests which would explain why it isn't working. Since this is utilizing the CLI perhaps the best thing to do is remove it as opposed to fixing it. @sagerb your input may be valuable here to understand just how difficult either is to get this passing.

…ile is invalid while CLI is also deprecated.
@sagerb
Copy link
Collaborator Author

sagerb commented Jan 27, 2025

your input may be valuable here to understand just how difficult either is to get this passing.

@dotNomad Thanks for diving in. I have just pushed a commit to remove that content type from the test and I have also updated that bogus string I added to be a bit more understandable.

We'll see if these changes now help the BAT tests to pass!

@dotNomad
Copy link
Collaborator

Thanks for diving in. I have just pushed a commit to remove that content type from the test and I have also updated that bogus string I added to be a bit more understandable.

Still failing. I think this is because the requirements file is also being generated for the fastapi-simple project and is empty given the dummy string.

remove bats test from pull-request CI jobs
@@ -50,3 +57,38 @@ func (s *defaultDependencyScanner) ScanDependencies(base util.AbsolutePath, pyth
}
return specs, nil
}

func GetRequirementsFilePath(base util.AbsolutePath) (util.RelativePath, bool, error) {
filePath := "requirements.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would include a comment here to note that this is used in inspection to find a requirements.txt

When reading this I was wondering why it wasn't using the config's package file filename briefly before looking at its usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've renamed the function to DoesDefaultRequirementsExist as well as adding a comment.

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

I did some changes to my machine to test some different cases:

I removed my global Python and this PR still worked perfectly based on the Python interpreter I had selected in the Python VS Code extension.

I also got the correct error if I didn't have the Python extension or Python globally.

I tested changing the selected Python interpreter in Positron and got the expected configurations.


I went through this with a pretty fine tooth comb, both for testing and the review itself. I test on VS Code, Positron, with and without Python environments selected, with an entrypoint at the workspace level and below.

This is a fantastic refactor. I only had one comment about adding a comment where I was a bit confused on the intention. 🚀

Comment on lines +37 to +40
# bats:
# needs: build
# secrets: inherit
# uses: ./.github/workflows/bats.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to do this in another PR for easier tracking. Maybe hold on merging this until we make an effort to remove.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#2558 was opened to remove these so we can solve the conflict with whatever merges first.

@sagerb sagerb merged commit b67034c into main Jan 28, 2025
13 checks passed
@sagerb sagerb deleted the sagerb-use-active-version-of-python-from-positron branch January 28, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants