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

Support optional python and r section attributes #2568

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/publisher/commands/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ func (cmd *DeployCmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContext)
return err
}
}
i := initialize.NewDefaultInitialize()
err = i.InitIfNeeded(absPath, cmd.ConfigName, ctx.Logger)
i := initialize.NewDefaultInitialize(nil, nil)
err = i.InitIfNeeded(absPath, cmd.ConfigName, ctx.Logger, false)
if err != nil {
return err
}
stateStore, err := state.New(absPath, cmd.AccountName, cmd.ConfigName, "", cmd.SaveName, ctx.Accounts, nil, false)
stateStore, err := state.New(absPath, cmd.AccountName, cmd.ConfigName, "", cmd.SaveName, ctx.Accounts, nil, false, nil, nil)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/publisher/commands/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ func (cmd *InitCommand) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContex
if cmd.ConfigName == "" {
cmd.ConfigName = config.DefaultConfigName
}
i := initialize.NewDefaultInitialize()
cfg, err := i.Init(absPath, cmd.ConfigName, cmd.Python, cmd.R, ctx.Logger)
i := initialize.NewDefaultInitialize(nil, nil)
cfg, err := i.Init(absPath, cmd.ConfigName, ctx.Logger, false)
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions cmd/publisher/commands/redeploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ func (cmd *RedeployCmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContex
}
ctx.Logger = events.NewCLILogger(args.Verbose, os.Stderr)

i := initialize.NewDefaultInitialize()
err = i.InitIfNeeded(absPath, cmd.ConfigName, ctx.Logger)
i := initialize.NewDefaultInitialize(nil, nil)
err = i.InitIfNeeded(absPath, cmd.ConfigName, ctx.Logger, false)
if err != nil {
return err
}
Expand All @@ -43,7 +43,7 @@ func (cmd *RedeployCmd) Run(args *cli_types.CommonArgs, ctx *cli_types.CLIContex
if err != nil {
return fmt.Errorf("invalid deployment name '%s': %w", cmd.TargetName, err)
}
stateStore, err := state.New(absPath, "", cmd.ConfigName, cmd.TargetName, "", ctx.Accounts, nil, false)
stateStore, err := state.New(absPath, "", cmd.ConfigName, cmd.TargetName, "", ctx.Accounts, nil, false, nil, nil)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/publisher/commands/requirements_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (cmd *CreateRequirementsCommand) Run(args *cli_types.CommonArgs, ctx *cli_t
if exists && !cmd.Force {
return errRequirementsFileExists
}
inspector, _ := inspect.NewPythonInspector(absPath, cmd.Python, ctx.Logger, nil, nil)
inspector, _ := inspect.NewPythonInspector(absPath, nil, ctx.Logger, nil)
reqs, incomplete, pythonExecutable, err := inspector.ScanRequirements(absPath)
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion cmd/publisher/commands/requirements_show.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func (cmd *ShowRequirementsCommand) Run(args *cli_types.CommonArgs, ctx *cli_typ
if err != nil {
return err
}
inspector, _ := inspect.NewPythonInspector(absPath, cmd.Python, ctx.Logger, nil, nil)
inspector, _ := inspect.NewPythonInspector(absPath, nil, ctx.Logger, nil)
reqs, incomplete, pythonExecutable, err := inspector.ScanRequirements(absPath)
if err != nil {
return err
Expand Down
38 changes: 30 additions & 8 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,30 @@ API_URL = "https://example.com/api"

## Python settings

The existence of the `python` section indicates a dependency on python for the deployment. All attributes are
optional, so the python section can be as simple as:

```toml
[python]
```

#### package_file

File containing package dependencies. The file must exist and be listed under 'files'. The default is 'requirements.txt'.
File containing package dependencies. This entry is optional and defaults to `requirements.txt`. When provided, the file must exist and be listed within `files`.

#### package_manager

Package manager that will install the dependencies. Supported values are `pip` and `none`. If package-manager is `none`, dependencies will not be installed.
Package manager that will install the dependencies. This entry is optional and defaults to `pip`. Supported values are `pip` and `none`. If package-manager is `none`, dependencies will not be installed.

#### version

Python version. The server must have a matching Python major/minor version in order to run the content.
Python version. The server must have a matching Python major/minor version in order to run the content. This entry is
optional and defaults to the version of the active Python interpreter.

Example:
By not supplying the python version attribute, the requested python version will "float" to the active version of Python being
used at the time of deployment. This may or may not be desired. If you need to "lock" it down, then provide the desired target value

#### Example: (with all attributes provided)

```toml
[python]
Expand All @@ -127,19 +138,30 @@ package_manager = "pip"

## R settings

The existence of the `r` section indicates a dependency on R for the deployment. All attributes are
optional, so the r section can be as simple as:

```toml
[r]
```

#### package_file

File containing package dependencies. This is usually `renv.lock`. The file must exist and be listed under 'files'.
File containing package dependencies. This entry is optional and defaults to the package file configured within the active R interpreter's `renv` installation (this is usually `renv.lock`). When provided, the file must exist and be listed within `files`.

#### package_manager

Package manager that will install the dependencies. Supported values are `renv` and `none`. If package-manager is `none`, dependencies will be assumed to be pre-installed on the server.
Package manager that will install the dependencies. This entry is optional and defaults to `renv`. Supported values are `renv` and `none`. If package-manager is `none`, dependencies will not be installed.

#### version

R version. The server will use the nearest R version to run the content.
R version. The server will use the nearest R version to run the content. This entry is
optional and defaults to the version of the active R interpreter.

Example:
By not supplying the r version attribute, the requested r version will "float" to the active version of R being
used at the time of deployment. This may or may not be desired. If you need to "lock" it down, then provide the desired target value

#### Example: (with all attributes provided)

```toml
[r]
Expand Down
35 changes: 26 additions & 9 deletions extensions/vscode/src/api/resources/Configurations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
ConfigurationInspectionResult,
} from "../types/configurations";

import { PythonExecutable, RExecutable } from "../../types/shared";

export class Configurations {
private client: AxiosInstance;

Expand All @@ -20,25 +22,41 @@ export class Configurations {
// 200 - success
// 404 - not found
// 500 - internal server error
get(configName: string, dir: string) {
get(
configName: string,
dir: string,
r: RExecutable,
python: PythonExecutable,
) {
Comment on lines +25 to +30
Copy link
Collaborator

@marcosnav marcosnav Jan 30, 2025

Choose a reason for hiding this comment

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

Could you elaborate more on why it is required to send an R and Python executables to get a configuration? Shouldn't the configuration response be what the existing file contains?

Copy link
Collaborator

Choose a reason for hiding this comment

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

but when it is passed around internally (and retrieved by the UX), that it have the current defaults inserted into the config object.

As a part of keeping this PR as narrow as possible we didn't change the behavior in the API and frontend.

Currently in main the defaults are inserted into the response even if the file doesn't have those attributes set (like the packageFile = 'requirements.txt settings). Rather than changing both the backend and frontend here we kept the API behavior the same.

Copy link
Collaborator

@marcosnav marcosnav Jan 30, 2025

Choose a reason for hiding this comment

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

I think I understand the reason of having the resulting config in the UI to have some defaults, but it feels confusing the way it is being handled.

This method is one of many that now accepts R and Python executables, but feels out of place, for example the getPythonPackages calling configurations/{name}/packages/python also accepts an R executable, and I find it confusing for future folks to have a method responsible to get python packages to accept an R executable.

The go method FromFile now accepts overriding defaults as activeRInterpreter and activePythonInterpreter, and we are now having a method that at first look suggests to load the configuration as it is on file, but consumers of it could find surprises from it.

My suggestion would be, to take a step back a bit and think how these signatures bring clarity or confusion to future folks working on this, specifically from the point that future folks might not have all the context on how this works or the history behind this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great point. The effort here was to avoid making changes on the frontend, but creating confusion by needing to pass in the Python and R executables everywhere is confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The go method FromFile now accepts overriding defaults as activeRInterpreter and activePythonInterpreter

To clarify, it's not overriding defaults but actually implementing them. The defaults come from the active interpreters, passed in at the time of the API request. Our current architecture requires all the levels of calls to pass these down into the deepest functions that need it. Unfortunately, that is very deep in this case.

I understand about the confusion. Let me think about this, and I'll signal for another review. Thanks.

const encodedName = encodeURIComponent(configName);
return this.client.get<Configuration | ConfigurationError>(
`/configurations/${encodedName}`,
{
params: { dir },
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}

// Returns:
// 200 - success
// 500 - internal server error
getAll(dir: string, params?: { entrypoint?: string; recursive?: boolean }) {
getAll(
dir: string,
r: RExecutable,
python: PythonExecutable,
params?: { entrypoint?: string; recursive?: boolean },
) {
return this.client.get<Array<Configuration | ConfigurationError>>(
"/configurations",
{
params: {
dir,
r: r.rPath,
python: python.pythonPath,
...params,
},
},
Expand Down Expand Up @@ -80,19 +98,18 @@ export class Configurations {
// 500 - internal server error
inspect(
dir: string,
python?: string,
r?: string,
r: RExecutable,
python: PythonExecutable,
params?: { entrypoint?: string; recursive?: boolean },
) {
return this.client.post<ConfigurationInspectionResult[]>(
"/inspect",
{
python,
r,
},
{},
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
...params,
},
},
Expand Down
9 changes: 5 additions & 4 deletions extensions/vscode/src/api/resources/ContentRecords.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
ContentRecord,
Environment,
} from "../types/contentRecords";
import { PythonExecutable, RExecutable } from "../../types/shared";

export class ContentRecords {
private client: AxiosInstance;
Expand Down Expand Up @@ -74,17 +75,15 @@ export class ContentRecords {
configName: string,
insecure: boolean,
dir: string,
r: RExecutable,
python: PythonExecutable,
secrets?: Record<string, string>,
r?: string,
python?: string,
) {
const data = {
account: accountName,
config: configName,
secrets: secrets,
insecure: insecure,
r: r,
python: python,
};
const encodedTarget = encodeURIComponent(targetName);
return this.client.post<{ localId: string }>(
Expand All @@ -93,6 +92,8 @@ export class ContentRecords {
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
Expand Down
16 changes: 14 additions & 2 deletions extensions/vscode/src/api/resources/Files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { AxiosInstance } from "axios";

import { ContentRecordFile, FileAction } from "../types/files";
import { Configuration } from "../types/configurations";
import { PythonExecutable, RExecutable } from "../../types/shared";

export class Files {
private client: AxiosInstance;
Expand All @@ -26,11 +27,22 @@ export class Files {
// 404 - configuration does not exist
// 422 - configuration files list contains invalid patterns
// 500 - internal server error
getByConfiguration(configName: string, dir: string) {
getByConfiguration(
configName: string,
dir: string,
r: RExecutable,
python: PythonExecutable,
) {
const encodedName = encodeURIComponent(configName);
return this.client.get<ContentRecordFile>(
`/configurations/${encodedName}/files`,
{ params: { dir } },
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}

Expand Down
61 changes: 51 additions & 10 deletions extensions/vscode/src/api/resources/Packages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
PythonPackagesResponse,
ScanPythonPackagesResponse,
} from "../types/packages";
import { PythonExecutable, RExecutable } from "../../types/shared";

export class Packages {
private client: AxiosInstance;
Expand All @@ -20,11 +21,22 @@ export class Packages {
// 409 - conflict (Python is not configured)
// 422 - package file is invalid
// 500 - internal server error
getPythonPackages(configName: string, dir: string) {
getPythonPackages(
configName: string,
dir: string,
r: RExecutable,
python: PythonExecutable,
) {
const encodedName = encodeURIComponent(configName);
return this.client.get<PythonPackagesResponse>(
`/configurations/${encodedName}/packages/python`,
{ params: { dir } },
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}

Expand All @@ -34,11 +46,22 @@ export class Packages {
// 409 - conflict (R is not configured)
// 422 - package file is invalid
// 500 - internal server error
getRPackages(configName: string, dir: string) {
getRPackages(
configName: string,
dir: string,
r: RExecutable,
python: PythonExecutable,
) {
const encodedName = encodeURIComponent(configName);
return this.client.get<GetRPackagesResponse>(
`/configurations/${encodedName}/packages/r`,
{ params: { dir } },
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}

Expand All @@ -48,25 +71,43 @@ export class Packages {
// 500 - internal server error
createPythonRequirementsFile(
dir: string,
python?: string,
r: RExecutable,
python: PythonExecutable,
saveName?: string,
) {
return this.client.post<ScanPythonPackagesResponse>(
"packages/python/scan",
{ python, saveName },
{ params: { dir } },
{ saveName },
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}

// Returns:
// 200 - success
// 400 - bad request
// 500 - internal server error
createRRequirementsFile(dir: string, saveName?: string, r?: string) {
createRRequirementsFile(
dir: string,
r: RExecutable,
python: PythonExecutable,
saveName?: string,
) {
return this.client.post<void>(
"packages/r/scan",
{ saveName, r },
{ params: { dir } },
{ saveName },
{
params: {
dir,
python: python.pythonPath,
r: r.rPath,
},
},
);
}
}
Loading
Loading