Skip to content
This repository has been archived by the owner on Feb 25, 2024. It is now read-only.

feat: add a debug mode for bentoctl #174

Closed
wants to merge 4 commits into from

Conversation

jjmachan
Copy link
Contributor

@jjmachan jjmachan commented Jul 2, 2022

Description

closes:

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #174 (eb8ed2c) into main (5db0df7) will decrease coverage by 0.46%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
- Coverage   60.27%   59.80%   -0.47%     
==========================================
  Files          24       24              
  Lines        1110     1117       +7     
==========================================
- Hits          669      668       -1     
- Misses        441      449       +8     
Flag Coverage Δ
unit-tests 59.80% <20.00%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bentoctl/cli/__init__.py 84.88% <ø> (-0.18%) ⬇️
bentoctl/utils/temp_dir.py 36.36% <12.50%> (-5.95%) ⬇️
bentoctl/docker_utils.py 26.38% <50.00%> (+1.03%) ⬆️
bentoctl/utils/__init__.py 57.14% <0.00%> (-14.29%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@@ -147,7 +146,6 @@ def build(
generate_deployable_container(
tag=local_docker_tag,
deployment_config=deployment_config,
cleanup=get_debug_mode(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we clean up when not in debug mode? :)

Comment on lines +57 to +61
"""
Calls the operator and generates the deployable. If in debug more the generated
deployable will be moved to the current directory for easier debugging.
"""
with TempDirectory(debug=get_debug_mode()) as dist_dir:
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving directory is a bit unnatural for debugging. Maybe preserving and printing the temp directory during debug should be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the behavior that we have now (except printing which folder it is). I prefer to have --debug put the intermediate deployable in my cur-dir since it speeds up debugging, but it is a personal preference.

Comment on lines 50 to 52
"""
if self.path is not None and os.path.exists(self.path):
shutil.rmtree(self.path, ignore_errors=ignore_errors)
Copy link
Contributor

Choose a reason for hiding this comment

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

If in debug mode, maybe we can print the temp dir instead of removing.

@jjmachan
Copy link
Contributor Author

closing this since we felt it would be better to print out the path for the intermediate bento instead of moving it to current path. #182 addresses the new change

@jjmachan jjmachan closed this Jul 21, 2022
@jjmachan jjmachan deleted the debug_mode branch July 21, 2022 06:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants