-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Dockerfile+ FastAPI for the CPU version added. #794
Conversation
…ved under ./model path
…et in the new run_api.py via the api query parameters
…variable from dockerfile.
@danikhani I reviewed the code 🙈 lint fails in pipeline... send PR to next branch |
.gitignore
Outdated
@@ -1,4 +1,5 @@ | |||
.idea | |||
models | |||
temp | |||
workdir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Dockerfile
Outdated
RUN git clone https://github.com/s0md3v/roop.git /roop | ||
RUN pip install -r roop/requirements-docker.txt | ||
|
||
CMD ["python", "/roop/run_api.py"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes
should be renamed to api.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMD should be double quotes. Quote from docs:
The exec form is parsed as a JSON array, which means that you must use double-quotes (“) around words not single-quotes (‘).
Renamed the file to api.py
docker-compose.yml
Outdated
version: '3.8' | ||
|
||
services: | ||
roopapi: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roop-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docker-compose.yml
Outdated
build: | ||
context: . | ||
dockerfile: Dockerfile | ||
restart: unless-stopped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docker-compose.yml
Outdated
restart: unless-stopped | ||
volumes: | ||
- roop-data:/roop | ||
- open-nsfw:/root/.opennsfw2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why that is exposed? all dependencies should be inside the container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to save the models in the volume. So if the container crashes, or the host pc shutsdown, the models remain and a new download is not required.
run_api.py
Outdated
roop.globals.reference_face_position = roop_parameters.reference_face_position | ||
roop.globals.reference_frame_number = roop_parameters.reference_frame_number | ||
roop.globals.similar_face_distance = roop_parameters.similar_face_distance | ||
roop.globals.temp_frame_format = roop_parameters.temp_frame_format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ENV to lock temp frame format, temp frame quality and video quality?
run_api.py
Outdated
app = FastAPI() | ||
|
||
class RoopModel(BaseModel): | ||
frame_processor: Optional[list] = ['face_swapper'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you extended the typing... but we now have repeated code from globals.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the Model class. Now all parameters are part of the method. Unfortunately, there is no certain way to avoid code duplication without changing a lot of root repo.
run_api.py
Outdated
execution_provider_list = ['cpu'] | ||
print(execution_provider_list) | ||
|
||
# setting paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for everything comment you have here... create a speaking method 😉
the body of the method is too long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Comments are removed. Moved some of the logic out of the method.
run_api.py
Outdated
target_file: UploadFile = File(...), | ||
roop_parameters: RoopModel = Depends() | ||
): | ||
# Removing the folder and its content if it already exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can probably merged with clear_temp()
anyway... it might not work when multiple user access the API as we delete data from multiple sessions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Solved the problem with having multiple user accessing the api by giving each request its own unique UUID4 path. This way the files related to an API call can get removed after the response has been sent. Other files related to other requests wont get removed.
openapi.json
Outdated
@@ -0,0 +1,282 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is generated output just for swagger or does fastapi actual consume it to define/validate the route response and request bodies?
if generated just for swagger it should not be committed... add to README how to generate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just for swagger. Its purpose was to allow users learn the API endpoint. The generation guid is added to the readme.
…er, since they still dont work properly
@danikhani I was thinking about your approach to move all models / weights to our folder. From what I understand, you like to provide all of them inside the image so you have a ready to start container and once the container stopped, the models / weights are persist.
That being said: I suggest to mount |
…ed uniq folder for each api request to save and edit files.
Totally with you on this one. I just wanted to avoid users download the models everytime they restart their pc/host. Now all the three models (roop, faceanaylser and nsfw2) are located in the model directory. Only model directory is being mounted as a volume. |
…readme. Using annotated instead of a class for the API configuration.
I applied the requested changes. After a review I will make anotherPR to the correct branch. |
I would also prefer to have a fresh PR as well as GitHub makes it hard to track outdates code fragments. Thanks for the effort so far... Reach out via Discord in case you have questions. |
RUN pip install -r roop/requirements.txt | ||
RUN pip install -r roop/requirements-docker.txt | ||
|
||
CMD ["python", "/roop/api.py"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMD ['python', '/roop/api.py']
RUN git clone https://github.com/s0md3v/roop.git /roop | ||
|
||
RUN pip install -r roop/requirements.txt | ||
RUN pip install -r roop/requirements-docker.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to requirements-headless.txt
and use it for docker and CI...
|
||
RUN git clone https://github.com/s0md3v/roop.git /roop | ||
|
||
RUN pip install -r roop/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove RUN pip install -r roop/requirements.txt
RUN apt-get update && apt-get install -y ffmpeg | ||
RUN pip install --upgrade pip | ||
|
||
RUN git clone https://github.com/s0md3v/roop.git /roop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look it to a tag
|
||
|
||
TEMP_FILES_PATH: str | ||
app = FastAPI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename app to API
roop.globals.target_path = target_saving_path_complete | ||
roop.globals.output_path = output_saving_path_complete | ||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this block to a method
update_status(str(e)) | ||
raise HTTPException(status_code=500, detail="Error while running roop.") | ||
|
||
background_tasks.add_task(remove_request_dir, path=request_dir_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dir = directory, rel = relative, e = exception ... do not use short variable names
|
||
|
||
@app.post("/start") | ||
async def image_file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method name should match the route in my opinion
roop.globals.output_video_encoder = output_video_encoder | ||
roop.globals.execution_threads = execution_threads | ||
|
||
request_uuid_str = str(uuid.uuid4()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to a method that says what is happening here
|
||
services: | ||
roop-api: | ||
environment: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting environment should be optional and therefore overrides to the defaults - you made it mandatory
|
||
@app.post("/start") | ||
async def image_file( | ||
background_tasks: BackgroundTasks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the core.py and api.py should use the same defaults - either we hardcode it in the globals or introduce a get_defaults() method there
|
||
|
||
@app.post("/start") | ||
async def image_file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the endpoint accepts images and videos
dockerfile: Dockerfile | ||
restart: always | ||
volumes: | ||
- roop-data:/roop/models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roop-models:/roop/models
|
||
PREDICTOR = None | ||
THREAD_LOCK = threading.Lock() | ||
MAX_PROBABILITY = 0.85 | ||
|
||
rel_path_to_nsfw_model = resolve_relative_path('../models/nsfw2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont make this global, resolve it in the methods
@danikhani I reviewed it once again - some checks from the https://github.com/s0md3v/roop/issues/632 are not fullfilt such as cuda support via an extra file. |
Next branch conflicts the original PR - I will add a Dockerfile by myself and serve the Gradio instead of FastAPI for the moment. |
Thanks to @henryruhs at #https://github.com/s0md3v/roop/issues/632#issue-1776243764 for giving a template of Dockerfiles. A fastAPI endpoint for starting a job and Dockerfile for starting the roop with the API endpoint in a container has been added.