Skip to content

[WIP] Remove conda yaml #1135

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Remove conda yaml #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 8 additions & 6 deletions lzy-api/src/main/proto/ai/lzy/v1/workflow/workflow.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ message Operation {
}

message PythonEnvSpec {
string yaml = 2; // Conda yaml to install before execution
repeated LocalModule localModules = 3;

message LocalModule {
string name = 1; // Name of module
string url = 2; // Url in storage to get module from
string python_version = 1; // 3.8, 3.9, ..., without patch
/* optional */ string pypi_index_url = 2; // Url of pypi index. If not set, using https://pypi.org/simple
Copy link
Contributor

@vhaldemar vhaldemar Oct 5, 2023

Choose a reason for hiding this comment

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

Maybe you should add extra_index_url (repeated) and find_links (repeated) for future?
look at pip install --help

Copy link
Collaborator

@futujaos futujaos Oct 12, 2023

Choose a reason for hiding this comment

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

May be we just should pass requirements.txt file contents, it supports all features and options of pip – https://pip.pypa.io/en/stable/reference/requirements-file-format/, including extra_index_url and others. Then we can just install packages with pip, without conda.

repeated PythonPackage requirements = 3; // Requirements to install
repeated string local_modules_urls = 4; // Urls in storage to get local modules from
Copy link
Collaborator

@futujaos futujaos Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Возможно, с целью написания дебаг-логов на сервере, локальный модуль представлять не только как URL, но как пару (URL, original_path_on_client). Но не знаю, насколько это необходимо. В целом есть ощущение, что под локальный модуль хочется зарезервировать целый message, чтобы в случае чего это было расширяемо. Например, сейчас у нас по урлам лежат архивы, но это может поменяться, и нужна будет какая-то мета про то, что лежит в урле.

Copy link
Contributor

@vhaldemar vhaldemar Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

also we could add some archive codec information here

Copy link
Collaborator Author

@ArtoLord ArtoLord Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I will add message LocalModule here


message PythonPackage {
string name = 1;
string version = 2;
Copy link
Collaborator

@futujaos futujaos Oct 10, 2023

Choose a reason for hiding this comment

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

we should also support things like package>=1.0,<2.0,!=1.5
and may be URL requirement (like Git)
so may be all requirements will be list of string, each string is requirement expression, like line in requirements.txt file
this could be non-secure though, we'll have to sanitize such expressions

}
}

Expand Down
2 changes: 1 addition & 1 deletion lzy/lzy-service/src/main/java/ai/lzy/service/config/ClientVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public class ClientVersions {
private static final Logger LOG = LogManager.getLogger(ClientVersions.class);
private static final Map<String, ClientVersionsDescription> versions = Map.of(
"pylzy", new ClientVersionsDescription(
/* minimal version */ new SemanticVersion(1, 14, 0),
/* minimal version */ new SemanticVersion(1, 15, 0),
Set.of()
)
);
Expand Down
12 changes: 9 additions & 3 deletions model/src/main/proto/ai/lzy/v1/common/env.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,15 @@ message EnvSpec {
}

message PythonEnv {
string name = 1;
string yaml = 2;
repeated LocalModule local_modules = 3;
string python_version = 1; // 3.8, 3.9, ..., without patch
Copy link
Contributor

@vhaldemar vhaldemar Oct 5, 2023

Choose a reason for hiding this comment

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

Why are you repeating 1:1 PythonEnvSpec here?

Copy link
Collaborator Author

@ArtoLord ArtoLord Oct 5, 2023

Choose a reason for hiding this comment

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

Because this is private api, and PythonEnvSpec is in public.

/* optional */ string pypi_index_url = 2; // Url of pypi index. If not set, using https://pypi.org/simple
repeated PythonPackage requirements = 3; // Requirements to install
repeated string local_modules_urls = 4; // Urls in storage to get local modules from

message PythonPackage {
string name = 1;
string version = 2;
}
}

message ProcessEnv {}
Expand Down