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

Analyze design of environment management #2288

Closed
ericsnowcurrently opened this issue Jul 31, 2018 · 7 comments
Closed

Analyze design of environment management #2288

ericsnowcurrently opened this issue Jul 31, 2018 · 7 comments
Assignees
Labels
area-terminal feature-request Request for new features or functionality

Comments

@ericsnowcurrently
Copy link
Member

While the extension supports discovery of virtual environments in a number of cases, there are still many ways in which they can remain undiscovered. I personally ran into this the other day using the Python-distributed venv, but it applies equally to virtualenv, pyenv, pipenv, anaconda, etc.

Automatic discovery is doable if we can keep in sync with the facilities for discovering environments of the different tools. However, there are limits to that, including the following contributing factors (see #2142):

  • environment variables
  • kind of shell (login, interactive, etc.)

Note that this relates closely to the issues of getting info from the terminal and of running subprocesses with the right environment variables.

@ericsnowcurrently ericsnowcurrently added feature-request Request for new features or functionality area-terminal needs decision labels Jul 31, 2018
@ericsnowcurrently
Copy link
Member Author

Also see #1479.

@brettcannon
Copy link
Member

Basically we need to know what kind of shell we use when we run subprocesses to know if we will pick up e.g. environment variables as set by folks in their .bashrc for pyenv (or LC_ALL for Black as another motivating example).

@brettcannon brettcannon changed the title Improve discovery of virtual environments. Analyze design of environment discovery/management Sep 19, 2018
@brettcannon brettcannon changed the title Analyze design of environment discovery/management Analyze design of environment management Sep 19, 2018
@DonJayamanne
Copy link

DonJayamanne commented Sep 20, 2018

Requirements:

  • Ability to discover different types of environments
  • Ability to create an environment for a particular type (create conda, create pip env, create pyenv)
  • Given an interpreter path, be able to determine the environment type for that interpreter
  • Provide the commands necessary to activate an environment for a given shell
    • Why would there be multiple commands?
  • Improved testability
  • Install modules into the environment
  • Discoverability of the code (e.g. all the related code goes in one place)

@DonJayamanne
Copy link

DonJayamanne commented Sep 20, 2018

Ideas

Note that these overlap substantially. So it isn't just a matter of choosing one. :)


Idea 1 (Single class for each type)

Have a single interface that does everything for a type of Python environment.
I.e. IEnvironment (concrete implementation for Conda, name CondaEnvironment class):

  • Listing conda environments
  • Activation of conda environments
  • Creation of conda environments
  • Identifying conda environments
    etc.

IEnvironment:

  • GetInterpreters() : Promise<PythonInterpreter[]>
    Gets a list of interpreters for the type of this implementation (e.g. Conda, Virtua, etc).
  • GetActivationScripts(shell: TerminalShellType): Promise<string[]>
    Gets the activation scripts.
  • GetInterpreterType(pythonPath:string): Promise<InterpretrerType|undefined>
    Gets the type of the interpreter.
  • GetInterpreterType(interpreter: PythonInterpreter): Promise<InterpreterType|undefined>
    Gets the type of the interpreter.
  • CreateEnvironment(name:string, path?:string): Promise
    Creates and environment with a given name and optionally in a specific path (not sure we need the second arg, to be determined).

Note:

  • Listing environments will return a simple object that defines an environment (with readonly attributes).
  • Much like a repository pattern.

Idea 2 (Composition)

Instead I'm for composition. I.e. IEnvironmentFinder or IEnvironmentProvider, IEnvironmentCreator, IEnvironmentActivator, etc.

Similar to what we have today, but refactor it to make it more maintainable.
For instance GlobalWorkspaceLocator might have to be broken up into GlobalPyEnvEnvLocator, GlobalVirtualEnvLocator, CondaLocator, etc...

Note:

  • Listing environments will return a simple object that defines an environment (with readonly attributes).
  • Much like a repository pattern, but instead of having a single interface to do everything we break it up.
  • I.e. we compose using lightweight interfaces..

Idea 3

Proposal by @d3r3kk that we go with interfaces such as IInterpreter that'll have members such as:

  • Type
  • Path
  • GetVersion()...
  • GetBitness()...
  • GetDisplayName()
  • GetInterpreters()
  • CreateInterpreter()

This is similar to the first approach but each instance of returning simple data objects we return objects that have can in turn do stuff.

@d3r3kk please feel free to update accordingly.


Idea 4

Like Option 2, but have one class per environment type that implements the granular interfaces.


Idea 5

3 tiers of info:

  1. distro -- e.g. who provided the Python executable (e.g. python.org, conda, ubuntu)
  2. environment
  3. interpreter

Here's a rough (and incomplete) sketch the should give you a sense of where I'd put info and functionality.

export interface IPythonDistribution {
    readonly name: NonEmptyString;
    readonly version: semver.SemVer;
    readonly publisher: NonEmptyString;
    readonly supportedVersions: ISupportedPythonVersions;
    readonly envs: IPythonEnvironments;

    /**
     * Add a new virtual environment using the distro.
     */
    createVenv(spec: IVenvSpec, version?: IPythonVersion): IPythonEnvironment;
}

export interface ISupportedPythonVersions {
    readonly [index: number]: ISupportedPythonVersion;
}

export interface ISupportedPythonVersion extends IPythonVersion {
    readonly installed: boolean;

    /**
     * Ensure that the Python version is installed.
     *
     * If that version of Python is directly usable then a new
     * environment is returned.  Otherwise null is returned.
     */
    ensureInstalled(): IPythonEnvironment | null;
}

//================================
// Python environments

export enum PythonEnvKind {
    Unknown,
    Global,
    Venv,
    VirtualEnv,
    PyEnv,
    PipEnv
}

export interface IPythonEnvironment {
    readonly name: NonEmptyString;
    readonly kind: PythonEnvKind;
    readonly distro: IPythonDistribution;
    readonly interpreter: IPythonInterpreter;
    readonly spec?: IVenvSpec;
    readonly packages: IPythonPackages;

    /**
     * Add a new virtual environment based on this env.
     */
    createVenv(IVenvSpec): IPythonEnvironment;

    // TODO: activation

    /**
     * Remove this environment.
     */
    destroy(): void;
}

export interface IPythonEnvironments {
    readonly [index: number]: IPythonEnvironment;
}

export interface IVenvSpec {
    readonly name: NonEmptyString;
    readonly dirname?: NonEmptyString;
}

//================================
// Python packages

export interface IPythonPackages {
    lookUp(string | IPythonPackageSpec): ISupportedPythonPackage;
    listAll(): IPythonPackage[];
}

export interface IPythonPackageSpec {
    readonly name: NonEmptyString;
    readonly version: semver.SemVer;
}

export interface ISupportedPythonPackage extends IPythonPackageSpec {
    install(): IPythonPackage[];
}

export interface IPythonPackage extends IPythonPackageSpec {
    uninstall(): void;
}

//================================
// Python interpreters

/**
 * A wrapper around a single Python interpreter.
 */
export interface IPythonInterpreter {
    readonly filename: NonEmptyString;
    readonly version: IPythonVersion;
    readonly env: IPythonEnvironment;
}

export enum PythonReleaseLevel {
    Alpha,
    Beta,
    Candidate,
    Final
}

export interface IPythonRelease {
    readonly releaseLevel: PythonReleaseLevel;
    readonly serial: int;
    toString(): string;
}

export interface IPythonVersion {
    readonly major: int;
    readonly minor: int;
    readonly micro?: int;
    readonly release?: IPythonRelease;
    toString(): string;
}

Idea 6

  • add implementations of the interfaces (e.g. from idea 5) to a global registry
    *add helper functions that pick the right implementation from the registry based on some criteria
  • replace existing environment-specific providers (spread throughout the code base) with generic providers that pull from the global registry for their specific needs (i.e. the providers/services have no knowledge of any specific environment; they are written strictly against the generic interfaces)

Idea 7

<TBD>

@brettcannon
Copy link
Member

I was actually thinking option 1 because as you outlined in the requirements you really only need 4 methods, but I'm not attached to the idea.

@DonJayamanne DonJayamanne self-assigned this Sep 20, 2018
@DonJayamanne
Copy link

@d3r3kk @ericsnowcurrently
Please could you review the above options & update if necessary.

@DonJayamanne
Copy link

Here's an incremental approach to solving this issue:

  • Restructure the code to improve discoverability
  • Review code structure and code
  • Start cycle again

@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-terminal feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

4 participants