feat: Adds "head" flag to `flow-run logs` #8003

ohadch · 2022-12-29T07:34:38Z

Adds a --head flag to the prefect flow-run logs <flow_run_id> command.

If head is provided, the first N logs are returned
Otherwise, all the logs are returned.

Example

prefect flow-run logs d6b76008-406c-4f69-a7b7-d1186e47a94d --head 2

Returns

2022-12-24 07:56:07.242 | INFO    | Flow run 'platinum-bat' - Log 0 from flow_run d6b76008-406c-4f69-a7b7-d1186e47a94d.
2022-12-24 07:56:07.243 | INFO    | Flow run 'platinum-bat' - Log 1 from flow_run d6b76008-406c-4f69-a7b7-d1186e47a94d.

Checklist

This pull request references any related issue by including "closes <link to issue>"
- If no issue exists and your change is not a small fix, please create an issue first.
This pull request includes tests or only affects documentation.
This pull request includes a label categorizing the change e.g. fix, feature, enhancement

- If head is provided, the first N logs are returned - Otherwise, all the logs are returned linked to issue PrefectHQ#7998

netlify · 2022-12-29T07:34:43Z

✅ Deploy Preview for prefect-orion ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`b32e1db`
🔍 Latest deploy log	https://app.netlify.com/sites/prefect-orion/deploys/63c6fc3da39969000eba7f85
😎 Deploy Preview	https://deploy-preview-8003--prefect-orion.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

ohadch · 2022-12-29T07:39:18Z

Although @serinamarie 's spec was 50 lines by default, I chose to return all of them as I found it a little more intuitive and convenient for the user. Please let me know if you think otherwise 🙏

serinamarie

Hi @ohadch, thanks for jumping on this so quickly!

When you say you return all logs by default when specifying the head option, I would expect that
prefect flow-run logs <flow_run_id> --head
would then return all logs, rather than running into the error: Option '--head' requires an argument. Also, I think it's likely that if a user specifies the head option, it's because they don't want all the logs by default, but I'll defer to @madkinsz on it.

In addition, I wonder if it makes sense to bother with making the creation of logs a fixture at some point.

src/prefect/cli/flow_run.py

ohadch · 2022-12-29T17:15:00Z

Hey @serinamarie ,

Sure 😄 Let's make it perfect.
I misunderstood your initial meaning before, but I think I got it now. Maybe let's try to clarify 😅

For now I support the following two cases:

prefect flow-run logs <uuid>                         // Returns all logs
prefect flow-run logs <uuid> --head N         // Returns the first N logs

You would also like the following to work:

prefect flow-run logs <uuid> --head           // Should return the first 50 logs

But unfortunately I can't seem to make it work, because I do not find how to make typer.Option() support a flag with a default value. I only succeed with declaring either a boolean flag, and then not being able to pass the optional value (aka 50), or declaring an option with a default value, but then not being able to display all logs when the user does not pass the --head parameter.

What do you think? Am I missing something? It is my first time working with typer, so maybe I missed something in the docs?

…n-logs-head

ohadch · 2022-12-29T18:03:33Z

@serinamarie Also, good idea regarding the fixture. Please have a look at that as well.

serinamarie

But unfortunately I can't seem to make it work, because I do not find how to make typer.Option() support a flag with a default value. I only succeed with declaring either a boolean flag, and then not being able to pass the optional value (aka 50), or declaring an option with a default value, but then not being able to display all logs when the user does not pass the --head parameter.

What do you think? Am I missing something? It is my first time working with typer, so maybe I missed something in the docs?

I agree, I don't think it's possible to have a flag support a default value here. We were thinking that an additional number param with a default of 20 would be an ideal way to ensure that if the head flag is set, we will receive 20 logs back, unless a user specifies the number themselves.

Example behavior:
prefect flow-run logs <flow_run_id> --head returns 20 logs
prefect flow-run logs <flow_run_id> --head -n 210 returns 210 logs

Then we can also use the -n param later on with --tail 😀

Also, the new fixture definitely makes the tests look more succinct! 🙏

- flow-run logs --head -> Returns 50 logs - flow-run logs --head --num-lines 10 -> returns 10 logs

ohadch · 2022-12-30T15:11:01Z

@serinamarie Thanks! This idea makes sense to me as well. I implemented it and used 20 lines for the default head value.

serinamarie

It's looking good! 😀 One other thing I noticed while testing this is that
prefect flow-run logs <flow_run_id> -n 5 displays all logs but we would like to display the first 5 logs (default sort), e.g. prefect flow-run logs <flow_run_id> -n 5 will display the oldest 5 logs in chronological order.

As another example, in the future with --reverse, prefect flow-run logs <flow_run_id> --reverse -n 5 would display the 5 most recent logs in reverse (newest first)

src/prefect/cli/flow_run.py

ohadch · 2023-01-04T14:33:00Z

Hey @serinamarie , it's a good catch, I wanted to discuss it but you already resolved my concern :) Now prefect flow-run logs <uuid> -n 10 for example indeed returns the first 10 results, using --head as a default.

serinamarie

Hi @ohadch, just a few small things to wrap this one up 🙂 Since a log can be multiple lines, I think --num_logs or --count would be a more descriptive long form of the -n command. Leaning toward num_logs. Sorry I didn't think on this sooner.

Additionally, I'm not sure we need to test Typer functionality, e.g. testing min=1 in test_when_num_lines_is_smaller_than_one_then_exit_with_error or the short forms of commands in test_h_and_n_shortcuts_for_head_and_num_lines. Is there some particular reason for it?

Thanks so much!

src/prefect/cli/flow_run.py

tests/cli/test_flow_run.py

ohadch · 2023-01-05T18:54:23Z

Regarding num-logs - I also think so. I thought that -l --limit may be also suitable, what do you think?

Regarding the tests - as it is more of an integration compared to a unit test, I do like to test all of the requirements regardless of the implementation method (in this case typer), but I sure can remove it if it is redundant in your opinion :) @serinamarie

ohadch · 2023-01-06T21:27:27Z

Hey @madkinsz , thanks for reviewing. Apparently this is one of "works on my machine" situations 😅. Not sure what causes it to fail at your end - can you please share the complete test run log?

zanieb · 2023-01-06T23:03:22Z

@ohadch You should be able to see the full logs in GitHub Actions, can you not?

I believe this is do to colored CLI output. @serinamarie and I will look into a fix.

ohadch · 2023-01-07T09:15:00Z

@madkinsz Sure, missed that. Seems like the coloring is a possible lead. On the other hand, as @serinamarie pointed out earlier, this test might be redundant anyway for this feature. What do you think?

zanieb · 2023-01-09T19:25:33Z

We disable colors with PREFECT_CLI_COLORS in normal output but since this validator is on the parameter typer is doing the output and typer does not respect that setting (we pass it to our rich console). We'll need to figure out how to disable typer's colored output during tests.

ohadch · 2023-01-10T15:50:48Z

@madkinsz I see the problem. My question is if that is a blocker in your opinion?

zanieb · 2023-01-11T18:12:03Z

@ohadch Yeah we cannot merge this with failing CI. You can add a pytest skip annotation to that test though and note that we need to disable colors.

tests/cli/test_flow_run.py

serinamarie

Hi @ohadch Can you skip that test for now?

Co-authored-by: Serina Grill <42048900+serinamarie@users.noreply.github.com>

ohadch · 2023-01-17T16:05:35Z

Hey @serinamarie , committed your suggestion for skipping the test. :)

serinamarie · 2023-01-17T19:54:11Z

Thanks a lot, @ohadch! 🎉

…er-builds-consolidation * 'main' of https://github.com/prefecthq/prefect: (34 commits) Minor API docstring formatting fixes, mostly lists (PrefectHQ#8196) Build multi-arch images for releases (PrefectHQ#7901) feat: Adds "head" flag to `flow-run logs` (PrefectHQ#8003) Updates to Automations and Notifications documentation (PrefectHQ#8140) Bump @prefecthq/vue-compositions from 1.2.3 to 1.2.9 in /orion-ui (PrefectHQ#8175) Bump eslint from 8.31.0 to 8.32.0 in /orion-ui (PrefectHQ#8174) Update Postgres tests to use Docker instead of GHA services (PrefectHQ#8125) Update `black` and `isort` versions in pre-commit (PrefectHQ#8167) Chore: Bump orion and prefect design to 1.2.0 (PrefectHQ#8163) Mark `test_process_streams_stderr` as flaky on Python 3.11 (PrefectHQ#8161) Add release notes for 2.7.8 (PrefectHQ#8160) Add some host_config arguments to DockerContainer (PrefectHQ#8033) Skip flaky test `test_dependent_events_in_two_loops_do_not_deadlock` (PrefectHQ#8159) Update jsonschema requirement from <4.0.0,>=3.2.0 to >=3.2.0,<5.0.0 (PrefectHQ#8152) Fix detection of conda installation in test (PrefectHQ#8149) Add `httpx.WriteError` to client retryable exceptions (PrefectHQ#8145) Add documentation for cache refreshes (PrefectHQ#8156) Add YouTube video to welcome page (PrefectHQ#8090) Add social links (PrefectHQ#8088) bump orion-design, release flow run timeline (PrefectHQ#8153) ...

Co-authored-by: ohadchaet <ohad.chaet@lynx.md> Co-authored-by: Michael Adkins <michael@prefect.io> Co-authored-by: Serina Grill <42048900+serinamarie@users.noreply.github.com>

feat: Adds "head" flag to flow-run logs

d212a38

- If head is provided, the first N logs are returned - Otherwise, all the logs are returned linked to issue PrefectHQ#7998

ohadch requested a review from zanieb as a code owner December 29, 2022 07:34

fix: Provides test with correct head value

8c4f2c1

serinamarie reviewed Dec 29, 2022

View reviewed changes

src/prefect/cli/flow_run.py Show resolved Hide resolved

src/prefect/cli/flow_run.py Outdated Show resolved Hide resolved

refactor: renames remaining_logs to user_specified_num_logs

85e94bd

ohadch requested review from serinamarie and removed request for zanieb December 29, 2022 17:18

ohadch and others added 3 commits December 29, 2022 19:19

Merge branch 'main' into flow-run-logs-head

988a342

feat: Uses a flow_run_factory fixture

70c7e0e

Merge remote-tracking branch 'origin/flow-run-logs-head' into flow-ru…

a668fed

…n-logs-head

serinamarie suggested changes Dec 29, 2022

View reviewed changes

ohadch and others added 3 commits December 30, 2022 16:51

Merge branch 'main' into flow-run-logs-head

3761095

refactor: uses constants

bef6e08

feat: Adds --num-lines flag

d59851d

- flow-run logs --head -> Returns 50 logs - flow-run logs --head --num-lines 10 -> returns 10 logs

ohadch requested a review from serinamarie December 30, 2022 15:09

ohad-lynx and others added 2 commits December 30, 2022 17:13

fix: Default head value is 20 lines

acdc5db

Merge branch 'main' into flow-run-logs-head

a2d2af6

serinamarie suggested changes Jan 3, 2023

View reviewed changes

src/prefect/cli/flow_run.py Outdated Show resolved Hide resolved

src/prefect/cli/flow_run.py Outdated Show resolved Hide resolved

serinamarie and others added 3 commits January 3, 2023 18:26

Merge branch 'main' into flow-run-logs-head

2d87101

fix: Limits num lines if -n is passed without --head

c0e0bf2

Merge branch 'main' into flow-run-logs-head

18f8187

ohadch requested a review from serinamarie January 5, 2023 14:37

serinamarie suggested changes Jan 5, 2023

View reviewed changes

Merge branch 'main' into flow-run-logs-head

57e4523

Merge branch 'main' into flow-run-logs-head

6a23a5d

ohadch requested a review from peytonrunyan as a code owner January 8, 2023 17:46

Merge branch 'main' into flow-run-logs-head

98995f2

ohadch requested a review from a team as a code owner January 10, 2023 15:50

zanieb added the enhancement An improvement of an existing feature label Jan 10, 2023

Merge branch 'main' into flow-run-logs-head

3f77df6

zanieb mentioned this pull request Jan 11, 2023

Disable typer help colors during test runs #8144

Closed

serinamarie reviewed Jan 17, 2023

View reviewed changes

tests/cli/test_flow_run.py Show resolved Hide resolved

serinamarie suggested changes Jan 17, 2023

View reviewed changes

ohadch and others added 2 commits January 17, 2023 18:03

Merge branch 'main' into flow-run-logs-head

f31f393

Update tests/cli/test_flow_run.py

cfdbc04

Co-authored-by: Serina Grill <42048900+serinamarie@users.noreply.github.com>

ohadch requested review from serinamarie and removed request for peytonrunyan January 17, 2023 16:04

zanieb approved these changes Jan 17, 2023

View reviewed changes

Merge branch 'main' into flow-run-logs-head

b32e1db

serinamarie approved these changes Jan 17, 2023

View reviewed changes

zanieb merged commit 84bf82d into PrefectHQ:main Jan 17, 2023

serinamarie mentioned this pull request Jan 19, 2023

Add Release Notes for 2.7.9 #8208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds "head" flag to `flow-run logs` #8003

feat: Adds "head" flag to `flow-run logs` #8003

ohadch commented Dec 29, 2022 •

edited by zanieb

Loading

netlify bot commented Dec 29, 2022 •

edited

Loading

ohadch commented Dec 29, 2022

serinamarie left a comment

ohadch commented Dec 29, 2022

ohadch commented Dec 29, 2022

serinamarie left a comment •

edited

Loading

ohadch commented Dec 30, 2022 •

edited

Loading

serinamarie left a comment •

edited

Loading

ohadch commented Jan 4, 2023

serinamarie left a comment

ohadch commented Jan 5, 2023 •

edited

Loading

ohadch commented Jan 6, 2023

zanieb commented Jan 6, 2023

ohadch commented Jan 7, 2023

zanieb commented Jan 9, 2023 •

edited

Loading

ohadch commented Jan 10, 2023

zanieb commented Jan 11, 2023

serinamarie left a comment

ohadch commented Jan 17, 2023

serinamarie commented Jan 17, 2023

feat: Adds "head" flag to flow-run logs #8003

feat: Adds "head" flag to flow-run logs #8003

Conversation

ohadch commented Dec 29, 2022 • edited by zanieb Loading

Example

Checklist

netlify bot commented Dec 29, 2022 • edited Loading

✅ Deploy Preview for prefect-orion ready!

ohadch commented Dec 29, 2022

serinamarie left a comment

Choose a reason for hiding this comment

ohadch commented Dec 29, 2022

ohadch commented Dec 29, 2022

serinamarie left a comment • edited Loading

Choose a reason for hiding this comment

ohadch commented Dec 30, 2022 • edited Loading

serinamarie left a comment • edited Loading

Choose a reason for hiding this comment

ohadch commented Jan 4, 2023

serinamarie left a comment

Choose a reason for hiding this comment

ohadch commented Jan 5, 2023 • edited Loading

ohadch commented Jan 6, 2023

zanieb commented Jan 6, 2023

ohadch commented Jan 7, 2023

zanieb commented Jan 9, 2023 • edited Loading

ohadch commented Jan 10, 2023

zanieb commented Jan 11, 2023

serinamarie left a comment

Choose a reason for hiding this comment

ohadch commented Jan 17, 2023

serinamarie commented Jan 17, 2023

feat: Adds "head" flag to `flow-run logs` #8003

feat: Adds "head" flag to `flow-run logs` #8003

ohadch commented Dec 29, 2022 •

edited by zanieb

Loading

netlify bot commented Dec 29, 2022 •

edited

Loading

serinamarie left a comment •

edited

Loading

ohadch commented Dec 30, 2022 •

edited

Loading

serinamarie left a comment •

edited

Loading

ohadch commented Jan 5, 2023 •

edited

Loading

zanieb commented Jan 9, 2023 •

edited

Loading