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

Install fastapi, pydantic and orjson during extension activation & bundle kedro-viz into extension #83

Merged
merged 8 commits into from
Sep 2, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Aug 28, 2024

Resolves #72

As pydantic and orjson depends on platform and python version, So it cant be packaged with extension so those dependencies are installed during extension activation in Kedro extension env (bundled/libs)

jitu5 added 4 commits August 23, 2024 14:59
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5 jitu5 marked this pull request as draft August 28, 2024 16:02
"py",
"--no-deps",
"--upgrade",
"kedro-viz",
Copy link
Contributor

Choose a reason for hiding this comment

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

For viz, do we have a particular version that we need to pin? (I think it's the next release version, so we need to wait until it is released, but I just want to confirm this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly it would be next release 10.0.0. later we willl pin it

src/extension.ts Outdated
export async function activate(context: vscode.ExtensionContext): Promise<void> {
const alreadyInstalled = context.globalState.get('dependenciesInstalled', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this globalState? Does that mean it only install if the dependency is not there? I couldn't find the code where this state is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a VSCode extension, globalState is an API provided by the VSCode framework that allows you to store and retrieve global state info. This state is persisted across VS Code sessions, it remains available even after VS Code is closed and reopened.

globalState does not have .set method.
Screenshot 2024-08-28 at 6 30 37 p m

Does that mean it only install if the dependency is not there?

No it just check if await installPythonDependencies(context); is called before with alreadyInstalled flag.

Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5
Copy link
Contributor Author

jitu5 commented Aug 28, 2024

@noklam you mentioned in slack

On client side (TS), we have a function that is callPythonScript
On Python side we keep install_dependencies as a executable script.

Could you please elaborate more this, so that I can make changes accordingly.

@noklam
Copy link
Contributor

noklam commented Aug 28, 2024

I think it's mainly just a refactoring, you have this now:

export async function installPythonDependencies(context: any): Promise<void> {
    return new Promise<void>(async (resolve, reject) => {
        const script = context.asAbsolutePath('bundled/tool/install_dependencies.py');
        const interpreterDetails = await getInterpreterDetails();
        const pythonPath = interpreterDetails['path'] && interpreterDetails['path'][0];

        cp.exec(`${pythonPath} ${script} ${EXTENSION_ROOT_DIR}`, (error, stdout, stderr) => {
            if (error) {
                console.error(stderr);
                reject(error);
            } else {
                console.log(stdout);
                resolve();
            }
        });
    });
}

I was thinking something more like this to make it slightly generic. Frankly it is fine to keep it as is as well.

export async function callPythonScript(context: any, path_to_script: str): Promise<void> {
    return new Promise<void>(async (resolve, reject) => {
        const script = context.asAbsolutePath(path_to_script);
        const interpreterDetails = await getInterpreterDetails();
        const pythonPath = interpreterDetails['path'] && interpreterDetails['path'][0];

        cp.exec(`${pythonPath} ${script} ${EXTENSION_ROOT_DIR}`, (error, stdout, stderr) => {
            if (error) {
                console.error(stderr);
                reject(error);
            } else {
                console.log(stdout);
                resolve();
            }
        });
    });
}

callPythonScript('bundled/tool/install_dependencies.py')

The reason I was thinking this because it is common for extension/LSP to call tools to perform different actions (think black or ruff for linting/formating)

jitu5 added 3 commits August 29, 2024 11:26
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@jitu5 jitu5 marked this pull request as ready for review August 29, 2024 10:55
@jitu5
Copy link
Contributor Author

jitu5 commented Aug 29, 2024

I think it's mainly just a refactoring, you have this now:

export async function installPythonDependencies(context: any): Promise<void> {
    return new Promise<void>(async (resolve, reject) => {
        const script = context.asAbsolutePath('bundled/tool/install_dependencies.py');
        const interpreterDetails = await getInterpreterDetails();
        const pythonPath = interpreterDetails['path'] && interpreterDetails['path'][0];

        cp.exec(`${pythonPath} ${script} ${EXTENSION_ROOT_DIR}`, (error, stdout, stderr) => {
            if (error) {
                console.error(stderr);
                reject(error);
            } else {
                console.log(stdout);
                resolve();
            }
        });
    });
}

I was thinking something more like this to make it slightly generic. Frankly it is fine to keep it as is as well.

export async function callPythonScript(context: any, path_to_script: str): Promise<void> {
    return new Promise<void>(async (resolve, reject) => {
        const script = context.asAbsolutePath(path_to_script);
        const interpreterDetails = await getInterpreterDetails();
        const pythonPath = interpreterDetails['path'] && interpreterDetails['path'][0];

        cp.exec(`${pythonPath} ${script} ${EXTENSION_ROOT_DIR}`, (error, stdout, stderr) => {
            if (error) {
                console.error(stderr);
                reject(error);
            } else {
                console.log(stdout);
                resolve();
            }
        });
    });
}

callPythonScript('bundled/tool/install_dependencies.py')

The reason I was thinking this because it is common for extension/LSP to call tools to perform different actions (think black or ruff for linting/formating)

@noklam created generic callPythonScript which can be used for other python script run.

@jitu5 jitu5 requested a review from noklam August 29, 2024 10:57
@noklam noklam changed the title Install fastapi, pydantic and orjson during extension activation Install fastapi, pydantic and orjson during extension activation & bundle kedro-viz into extension Aug 29, 2024
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Updated the title, all looks good!

@noklam
Copy link
Contributor

noklam commented Sep 2, 2024

Is there anything left to be done here? If not I can merge the PR.

@jitu5
Copy link
Contributor Author

jitu5 commented Sep 2, 2024

Is there anything left to be done here? If not I can merge the PR.

@noklam Most of the things are done under this PR. we can merge now.

@noklam noklam merged commit 32c6f17 into main Sep 2, 2024
2 checks passed
@noklam noklam deleted the viz/viz-py-test branch September 2, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide to vendor kedro-viz or require user to install
2 participants