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

Documentation #11

Merged
merged 6 commits into from
Oct 8, 2024
Merged

Documentation #11

merged 6 commits into from
Oct 8, 2024

Conversation

dgbaenar
Copy link
Contributor

  • Adding documentation for evaluate retrieval and generation
  • Readme updated
  • Docker compose updated and working

@dgbaenar dgbaenar added the enhancement New feature or request label Sep 19, 2024

RUN pip install --no-cache-dir -r requirements.txt

EXPOSE 8000

CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I placed it in the docker compose so that the run was not executed when the container is built. Now the command is run after all the containers are up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this file should be commited to git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

File removed from the PR

ports:
- "8000:8000"
volumes:
- ./data:/app/data
- ./data:/app/app/data
Copy link
Contributor

Choose a reason for hiding this comment

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

why double app?

Copy link
Contributor Author

@dgbaenar dgbaenar Oct 7, 2024

Choose a reason for hiding this comment

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

Because of the absolute imports I use during development and run uvicorn from the root of the project. Since the project is inside app, and the uvicorn command runs inside the workdir I specify in the dockerfile, I have to include the app folder for the uvicorn to work app.main:app --host 0.0.0.0.0 --port 8000 --reload

Copy link
Contributor

Choose a reason for hiding this comment

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

But you could set the workdir to / in the Dockerfile before running uvicorn, so that it is run the same way as during development

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and removed the double app folder

-t 10
--host 0.0.0.0
--port 8080
-m models/Meta-Llama-3.1-8B-Instruct-F16-Q5_K_M.gguf -n 450 -c 2048 -t 10 --host 0.0.0.0 --port 8080
Copy link
Contributor

Choose a reason for hiding this comment

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

-t 10 should be documented, and I am not sure it is really beneficial without parallel execution. Have you tested if there is a latency difference with t=4 for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the -t explanation to documentation in the main README.md. Also specified that the parallel parameter -np is optional and also added the explanation of how to use it

@dgbaenar dgbaenar merged commit c13a6ca into main Oct 8, 2024
2 checks passed
@dgbaenar dgbaenar deleted the documentation branch October 31, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants