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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify --save-profiling-data to accept File Path(Ploomber Engine CLI) #78

Conversation

tonykploomber
Copy link
Contributor

@tonykploomber tonykploomber commented May 29, 2023

Describe your changes

Now save_profiling_data can take Path string, also the existing bool usage will be supported as well.

Changes:

Screenshot 2023-05-31 at 3 27 45 PM

Issue number

Closes #65

Checklist before requesting a review


馃摎 Documentation preview 馃摎: https://ploomber-engine--78.org.readthedocs.build/en/78/

@tonykploomber tonykploomber marked this pull request as draft May 29, 2023 20:01
@tonykploomber tonykploomber marked this pull request as ready for review May 31, 2023 19:30
src/ploomber_engine/cli.py Outdated Show resolved Hide resolved
src/ploomber_engine/execute.py Outdated Show resolved Hide resolved
ValueError,
),
(
float(123.0),
Copy link
Contributor

Choose a reason for hiding this comment

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

im confused about these and the values below. we're testing a CLI, so all the inputs must be serialized to strings. how come we're passing floats, dictionaries and tuples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the jupyter user might execute something like this:

_ = execute_notebook(
    "notebook.ipynb", "output.ipynb", profile_memory=True, save_profiling_data=float(123.0))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If execute execute_notebook directly, it's possible we pass non-string

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I understand. but this is testing the CLI. either this is a problem with runner.invoke or the values are being serialized to strings anyway.

i think it's best to parameterized this test over different strings then. because the other values are not representative of a real CLI use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got what you means, and this test case is to test on running on CLI

Remove other datatypes in 85b0110

But LMK if you want me to test other string

src/ploomber_engine/execute.py Show resolved Hide resolved
@tonykploomber tonykploomber changed the title 65 modify save profiling data to accept file pathploomber engine cli Modify --save-profiling-data to accept File Path(Ploomber Engine CLI) Jun 1, 2023
tests/test_execute.py Show resolved Hide resolved
@edublancas edublancas merged commit 27fbc10 into main Jun 5, 2023
16 checks passed
@edublancas edublancas deleted the 65-modify-save-profiling-data-to-accept-file-pathploomber-engine-cli branch June 5, 2023 18:17
@avivajpeyi
Copy link
Contributor

Hey! this is great, is there a planned pypi release date for 0.0.29dev?

@edublancas
Copy link
Contributor

edublancas commented Jul 17, 2023

I forgot to make the release, just did it!

it ended up being 0.0.30 because there was some issue with the package's metadata (0.0.29 failed to upload to PyPI)

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.

Modify --save-profiling-data to accept File Path(Ploomber Engine CLI)
3 participants