fix: Disable model-serving by default #207
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested.
- Reviewed the entire pull request up to 1e231cd
- Looked at
65lines of code in3files - Took 1 minute and 34 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
2additional comments because they didn't meet confidence threshold of50%.
1. agents-api/docker-compose.yml:10:
- Assessed confidence :
66% - Grade:
0% - Comment:
The model-serving service has been removed from the dependencies. If the agents-api service relies on the model-serving service, this could cause issues when the agents-api service starts before the model-serving service is ready. - Reasoning:
The model-serving service has been removed from the dependencies of the agents-api service. If the agents-api service relies on the model-serving service to be running, this could cause issues.
2. docker-compose.yml:10:
- Assessed confidence :
66% - Grade:
0% - Comment:
The model-serving service has been commented out. If other services depend on the model-serving service, this could cause issues when those services start before the model-serving service is ready. - Reasoning:
The model-serving service has been commented out in the main docker-compose file. If other services depend on the model-serving service, this could cause issues.
Workflow ID: wflow_iqw2yb9AzgMxC7el
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 1e231cd
- Looked at
20lines of code in1files - Took 1 minute and 41 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_NEOSDyeb8gFcQNgf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 3a1e622
- Looked at
36lines of code in1files - Took 3 minutes and 0 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
2additional comments because they didn't meet confidence threshold of50%.
1. .github/workflows/push-to-hub.yml:23:
- Assessed confidence :
100% - Grade:
0% - Comment:
The PR description mentions that the workflow should trigger on any push event, not just those to thedevbranch. However, theondirective here is not modified to reflect this. Please ensure that theondirective is correctly configured to trigger the workflow on any push event. - Reasoning:
The PR description mentions that the changes are made to trigger the workflow on any push event, not just those to thedevbranch. However, the changes in the PR do not reflect this. Theondirective is not modified and the lines specifying thedevbranch are not commented out. This seems to be a logical bug.
2. .github/workflows/push-to-hub.yml:27:
- Assessed confidence :
100% - Grade:
0% - Comment:
The cache is being stored in the/tmpdirectory, which is not persistent across different workflow runs. This means that the cache will be lost after the workflow run completes, defeating the purpose of caching. Consider using GitHub's cache action to cache the Docker layers. - Reasoning:
The PR introduces caching for Docker layers, which is a good practice for improving build times. However, the cache is being stored in the/tmpdirectory, which is not persistent across different workflow runs. This means that the cache will be lost after the workflow run completes, defeating the purpose of caching. A better approach would be to use GitHub's cache action to cache the Docker layers.
Workflow ID: wflow_Dq3q4MfTf19MHr8g
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 91c0198
- Looked at
64lines of code in1files - Took 2 minutes and 28 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of50%.
1. .github/workflows/push-to-hub.yml:8:
- Assessed confidence :
33% - Comment:
The on directive in the workflow file is commented out and replaced with on: [push], which means that the workflow will indeed trigger on any push event. This is consistent with the PR description, but the commented out lines could be removed for clarity. - Reasoning:
The PR description mentions that the GitHub Actions workflow has been modified to trigger on any push event. However, the on directive in the workflow file is commented out and replaced with on: [push], which means that the workflow will indeed trigger on any push event. This is consistent with the PR description, but the commented out lines could be removed for clarity.
2. .github/workflows/push-to-hub.yml:18:
- Assessed confidence :
66% - Grade:
0% - Comment:
The model-serving service is commented out in the matrix strategy. This means that the model-serving service will not be built and pushed to Docker Hub. If this is the intended behavior, the PR title and description should be updated to reflect this change. - Reasoning:
The PR title mentions 'Disable model-serving by default' but the changes in the PR do not reflect this. The model-serving service is commented out in the matrix strategy for the GitHub Actions workflow. This means that the model-serving service will not be built and pushed to Docker Hub. If this is the intended behavior, the PR title and description should be updated to reflect this change.
3. .github/workflows/push-to-hub.yml:38:
- Assessed confidence :
66% - Grade:
0% - Comment:
The cache-from and cache-to directives in the Docker build-push-action are set to type=gha, which means that the cache is stored in the GitHub Actions environment and not in /tmp/.buildx-cache as mentioned in the PR description. Please clarify this discrepancy. - Reasoning:
The PR description mentions that Docker layer caching has been introduced to speed up the build process. However, the cache-from and cache-to directives in the Docker build-push-action are set to type=gha, which means that the cache is stored in the GitHub Actions environment and not in /tmp/.buildx-cache as mentioned in the PR description. This discrepancy should be clarified.
Workflow ID: wflow_iA6yzHC7DZHCroHJ
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 342acf0
- Looked at
18lines of code in1files - Took 1 minute and 56 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. .github/workflows/push-to-hub.yml:38:
- Assessed confidence :
100% - Grade:
0% - Comment:
The PR description mentions the introduction of Docker layer caching to speed up the build process, but the changes in this file do not reflect that. The caching mechanism appears to be unchanged with 'type=gha'. Please ensure the changes align with the PR description. - Reasoning:
The PR description mentions that Docker layer caching has been introduced to speed up the build process. However, looking at the diff, it seems like the caching mechanism has not been changed. The cache-from and cache-to fields are still set to 'type=gha', and the commented out lines that would have enabled registry caching are still commented out. This is a discrepancy between the PR description and the actual changes made.
Workflow ID: wflow_9zEHwdnNjTdsGCa8
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 13b1515
- Looked at
29lines of code in1files - Took 1 minute and 44 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. .github/workflows/push-to-hub.yml:20:
- Assessed confidence :
50% - Comment:
Themodel-servingservice has been commented out. This means that the Docker image for this service will not be built and pushed to Docker Hub. If other services depend on this, it could potentially lead to issues. Please confirm if this is the intended behavior. - Reasoning:
The PR description mentions that themodel-servingservice has been disabled by default. Looking at the diff, it seems that themodel-servingservice has been commented out in the GitHub Actions workflow file. This means that the Docker image for themodel-servingservice will not be built and pushed to Docker Hub when a push event is triggered. This could potentially lead to issues if other services depend on themodel-servingservice and its Docker image. I would suggest confirming whether this is the intended behavior or not.
Workflow ID: wflow_edZ6mh6xNpVosDlx
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Signed-off-by: Diwank Singh Tomer <[email protected]>
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 8b1ba62
- Looked at
138lines of code in1files - Took 2 minutes and 15 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. .github/workflows/push-to-hub.yml:27:
- Assessed confidence :
100% - Grade:
40% - Comment:
Consider storing the Docker Hub username as a GitHub secret and referencing it in the workflow to increase security and flexibility.
username: "${{ secrets.DOCKER_HUB_USERNAME }}"
- Reasoning:
The PR author has hardcoded the Docker Hub username 'julepai' into the workflow. This could be a potential security risk and it also reduces the flexibility of the workflow. It would be better to store the Docker Hub username as a GitHub secret and reference it in the workflow.
Workflow ID: wflow_Wjep03x6Bb4ud1Zh
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on b0a2053
- Looked at
40lines of code in2files - Took 1 minute and 51 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /docker-compose.yml:6:
- Assessed confidence :
100% - Grade:
0% - Comment:
The PR description mentions that themodel-servingservice is disabled by default, but it is included in thedocker-compose.ymlfile. Please clarify or correct this. - Reasoning:
The PR description mentions that themodel-servingservice is disabled by default. However, in thedocker-compose.ymlfile, themodel-servingservice is included. This seems contradictory and could potentially lead to unexpected behavior.
Workflow ID: wflow_EoRyHGRkTrqPeZ2t
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
This was referenced Apr 18, 2024
This was referenced Apr 26, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR modifies the GitHub Actions workflow, introduces Docker layer caching, and disables the
model-servingservice by default indocker-compose.yml.Key points:
/.github/workflows/push-to-hub.ymlto trigger on any push eventmodel-servingservice by default indocker-compose.yml/tmp/.buildx-cacheand keyed by runner OS and commit SHAGenerated with ❤️ by ellipsis.dev