-
Notifications
You must be signed in to change notification settings - Fork 26
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
[SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use pathlib.Path module for checking cache size #1536
[SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use pathlib.Path module for checking cache size #1536
Conversation
…networks/schematic into schematic-30-add-version-to-cli
output = subprocess.run(command, capture_output=True, check=False).stdout.decode( | ||
"utf-8" | ||
total_size = sum( | ||
f.stat().st_size for f in Path(directory).rglob("*") if f.is_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.
There may be some minor discrepancies between how this would operate on windows, mac, and ubuntu
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 doesn't work on WSL2 ubunutu:
Traceback (most recent call last):
File "/home/bfauble/BryansGreatWorkspace/schematic/junk.py", line 7, in <module>
print(check_synapse_cache_size())
File "/home/bfauble/BryansGreatWorkspace/schematic/schematic/utils/general.py", line 141, in check_synapse_cache_size
total_size = sum(
File "/home/bfauble/BryansGreatWorkspace/schematic/schematic/utils/general.py", line 141, in <genexpr>
total_size = sum(
File "/home/bfauble/.pyenv/versions/3.10.14/lib/python3.10/pathlib.py", line 1047, in rglob
for p in selector.select_from(self):
File "/home/bfauble/.pyenv/versions/3.10.14/lib/python3.10/pathlib.py", line 405, in select_from
if not is_dir(parent_path):
File "/home/bfauble/.pyenv/versions/3.10.14/lib/python3.10/pathlib.py", line 1305, in is_dir
return S_ISDIR(self.stat().st_mode)
File "/home/bfauble/.pyenv/versions/3.10.14/lib/python3.10/pathlib.py", line 1097, in stat
return self._accessor.stat(self, follow_symlinks=follow_symlinks)
PermissionError: [Errno 13] Permission denied: '/root/.synapseCache'
from schematic.utils.general import check_synapse_cache_size
print(check_synapse_cache_size())
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 guess before it probably wouldn't have either since the default is to use /root
. It does work though with how it's implemented in code:
from schematic.store.synapse import SynapseStorage
from schematic.utils.general import check_synapse_cache_size
synapse_store = SynapseStorage()
print(check_synapse_cache_size(synapse_store.root_synapse_cache))
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.
Ya, I think the default being root is an issue here, it should work if you set it to any other folder you had access to...
That said, I think there was a reason why the default was set to /root/.synapseCache
) -> None: | ||
mock_synapse_cache_dir, _, _ = create_temp_query_file | ||
disk_size = check_synapse_cache_size(mock_synapse_cache_dir) | ||
|
||
# For some reasons, when running in github action, the size of file changes. |
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.
du -h
was not completely accurate as it would also potentially gather the bytes that a 'folder' takes up which we don't necessarily care too much about.
This change also ensures the results are the same on github actions
("", [1024, 2048], 3072), # Directory with multiple files (1 KB + 2 KB) | ||
], | ||
) | ||
def test_check_synapse_cache_size(mocker, directory, file_sizes, expected_size): |
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.
There are separate tests in the test_utils.py
, should I move them here?
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.
Yes please, those under class TestGeneral:
look like good candidates to move over here
@@ -24,6 +25,7 @@ | |||
# invoke_without_command=True -> forces the application not to show aids before losing them with a --h | |||
@click.group(context_settings=CONTEXT_SETTINGS, invoke_without_command=True) | |||
@click_log.simple_verbosity_option(logger) | |||
@click.version_option(version=__version__, prog_name="schematic") |
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.
Double check that this works in a docker container. I ran into issues using __version__
where on my local machine it worked fine, but in docker it would crash because the package was not found.
Related to 778bf54#diff-cf3cc0451d73d8b13adca0706b2ee953089f7fe25000b03615bf14dd2a7220ca - I ended up removing the code to grab the version out of this because of the error.
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.
Quality Gate passedIssues Measures |
Problem
du -h
system to check for cache size which isn't accurate AND may fail on windowssolution
Add version to cli
This confirms that it will pull from the pyproject.toml, so that needs to be updated