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

Run codespell using pre-commit #602

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Run codespell using pre-commit #602

merged 1 commit into from
Apr 24, 2024

Conversation

khatwanimohit
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@gobbleturk gobbleturk left a comment

Choose a reason for hiding this comment

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

More proffesional every day. Soon maybe even professional.

@khatwanimohit khatwanimohit force-pushed the mohit/codespell branch 4 times, most recently from 25b8ff2 to c679e20 Compare April 19, 2024 20:03
@khatwanimohit khatwanimohit force-pushed the mohit/codespell branch 2 times, most recently from 5217b86 to 38d285e Compare April 23, 2024 00:16
@copybara-service copybara-service bot merged commit 35f2b8a into main Apr 24, 2024
8 checks passed
@copybara-service copybara-service bot deleted the mohit/codespell branch April 24, 2024 17:54
@@ -38,13 +38,12 @@ RUN mkdir -p /deps
# Set the working directory in the container
WORKDIR /deps

# Copy necessary build files to docker container
COPY setup.sh requirements.txt constraints_gpu.txt /deps/
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is done so that when we are making changes independent of dependencies, we won't do the installation step again, which can take >2m. Is there a specific concern to overrule it? @khatwanimohit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setup.sh now includes running the pre-commit install command which installs the pre-commit hooks in the user's git repo. That command needs access to .pre-commit-config.yaml file

@@ -195,7 +195,8 @@ fi
# Install dependencies from requirements.txt
cd $run_name_folder_path && pip install --upgrade pip
if [[ "$MODE" == "pinned" ]]; then
pip3 install -r requirements.txt -c constraints_gpu.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think adding -U here goes against the idea of pinned build. Previously, the versions installed will be the function of the base image (which is pinned) and reqs and constraints. Now it could potentially be time-dependent if we happen to miss to capture the constraints in the constraints file. WDYT? @khatwanimohit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand, we can remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants