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

[SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use pathlib.Path module for checking cache size #1536

Conversation

thomasyu888
Copy link
Member

@thomasyu888 thomasyu888 commented Nov 9, 2024

Problem

  • It was not easy to surface the version of the cli or library used
  • we were using du -h system to check for cache size which isn't accurate AND may fail on windows

solution

Add version to cli

schematic -h
Usage: schematic [OPTIONS] COMMAND [ARGS]...

  Command line interface to the `schematic` backend services.

Options:
  -v, --verbosity LVL  Either CRITICAL, ERROR, WARNING, INFO or DEBUG
  --version            Show the version and exit.
  -h, --help           Show this message and exit.

Commands:
  manifest  Sub-commands with Manifest Generation utilities/methods.
  model     Sub-commands for Metadata Model related utilities/methods.
  schema    Sub-commands for Schema related utilities/methods.
  viz       Sub-commands for Visualization methods.

schematic --version
schematic, version 24.12.1
import schematic
schematic.__version__
Out[5]: '24.12.1'

This confirms that it will pull from the pyproject.toml, so that needs to be updated

docker build . -t test
docker run test schematic --version
schematic, version 24.12.1

@thomasyu888 thomasyu888 marked this pull request as ready for review November 13, 2024 02:34
@thomasyu888 thomasyu888 requested a review from a team as a code owner November 13, 2024 02:34
@thomasyu888 thomasyu888 marked this pull request as draft November 13, 2024 02:57
@thomasyu888 thomasyu888 changed the title [SCHEMATIC-30] Add version to click cli [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli Nov 13, 2024
@thomasyu888 thomasyu888 changed the title [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use os module for checking cache size Nov 13, 2024
@thomasyu888 thomasyu888 marked this pull request as ready for review November 13, 2024 03:37
@thomasyu888 thomasyu888 changed the title [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use os module for checking cache size [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use Path module for checking cache size Nov 13, 2024
@thomasyu888 thomasyu888 changed the title [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use Path module for checking cache size [SCHEMATIC-30, SCHEMATIC-200] Add version to click cli / use pathlib.Path module for checking cache size Nov 13, 2024
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()
Copy link
Member Author

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

Copy link
Collaborator

@BryanFauble BryanFauble Nov 13, 2024

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())

Copy link
Collaborator

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))

Copy link
Member Author

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.
Copy link
Member Author

@thomasyu888 thomasyu888 Nov 13, 2024

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):
Copy link
Member Author

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?

Copy link
Collaborator

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")
Copy link
Collaborator

@BryanFauble BryanFauble Nov 13, 2024

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.

Copy link
Collaborator

@BryanFauble BryanFauble Nov 13, 2024

Choose a reason for hiding this comment

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

For this I had just used docker build . and did a docker run on the built image. If it starts up we would just want to confirm that we could print the version to console.

I had tried the same things you did here:

a096639

2e3b3cf

b718f85

@thomasyu888 thomasyu888 changed the base branch from develop to add-otel-flask-as-non-extra-dep November 13, 2024 21:51
Copy link

sonarcloud bot commented Nov 14, 2024

@thomasyu888 thomasyu888 deleted the branch add-otel-flask-as-non-extra-dep November 14, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants