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

Added automatic requirements installation from docstring #162

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DarkIceXD
Copy link

@DarkIceXD DarkIceXD commented Jul 16, 2024

This makes it so that the requirements in the python docstring get installed before executing the pipeline. Fixes #151 .
The process goes like:

  • load python file
  • parse using ast
  • get the docstring from ast
  • find the requirements section in the docstring using regex (re)
  • create a process using subprocess.Popen and pip install the dependencies

Everything gets done before the script executes for the first time.

To test this:

  • first run pip uninstall numpy
  • then add this test.py to your pipelines directory or upload using swagger UI (http://localhost:9099/docs)
"""
title: Test pipline
requirements: numpy
"""

from typing import List, Union, Generator, Iterator
import numpy

class Pipeline:
    def __init__(self):
        print(numpy)
    
    def pipe(
        self, user_message: str, model_id: str, messages: List[dict], body: dict
    ) -> Union[str, Generator, Iterator]:
        return "hello world"

@Marenz
Copy link
Contributor

Marenz commented Jul 18, 2024

Looks nice. How does it behave in case a dependency isn't found or some other error?

Sidenote: there is the uv pip drop-in alternative which is (without exaggeration) x10 faster, if there would be an option to use it instead, that would be awesome. Though make it configurable might be a bit extra work.

Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

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

Yeah, that is def. better now

@DarkIceXD
Copy link
Author

DarkIceXD commented Jul 19, 2024

Looks nice. How does it behave in case a dependency isn't found or some other error?

it doesnt care. notice line 130 one could change the _, _ = ... to stdout, stderr = ... and get the output or get the return code from the process but i figured that we might as well try to run the script anyway and see if it fails (right now it just runs the script with missing dependencies aswell).

Sidenote: there is the uv pip drop-in alternative which is (without exaggeration) x10 faster, if there would be an option to use it instead, that would be awesome. Though make it configurable might be a bit extra work.

i never used it. we could add uv in the requirements.txt and then just use uv pip. are there any drawbacks?

@DarkIceXD
Copy link
Author

also i could remove line 84-105 and line 120-124 in start.sh as it isnt needed anymore once the pull request gets merged

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.

Dependencies not being pulled
2 participants