Feature/set preview limit by xtomflo · Pull Request #19 · sungchun12/fst

xtomflo · 2023-04-01T23:16:27Z

Added the set preview limit, worked as far as I've tested it

Removed redundant call of setup_logger() as it was resulting in triplicated logging output. ie everything that was logged after it, was printed 3 times.

…gger call in start_directory_watcher function feat(main.py): add logger instantiation in main function

sungchun12 · 2023-04-02T00:34:11Z

Cool I'll take a deeper look at this on Monday! Thanks for the PR. Love that we're evolving this together :)

sungchun12 · 2023-04-02T00:55:54Z

@@ -22,7 +24,6 @@ def start_streamlit() -> None:
    subprocess.run(["streamlit", "run", streamlit_app_path])

 def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:


Suggested change

def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:

def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:

setup_logger(log_queue)

This is needed or else none of the logs show up.

before

after

Show me a screenshot where you're seeing duplicate logs

My gut tells me this is a quirk using WSL. See if you can run this on linux using the github codespace.

Which logs do you have in mind?
In the screenshots you're showing different parts of the output in different scenarios, one is failure and one is success.
If it's about 'dbt build' logs (I assume it's this one), then in the second one they're also visible ("Completed successfully").

If it's about the query & build time, they're not visible in case of failure independent of whether 'setup_logger' is added here or not.

Here with the setup_logger() in start_directory_watcher ( duplicated output)

Here without - current state of the PR:

That's so interesting. I've never seen the duplicate logs before in my setup. This is affirming my point that something may be going on in WSL

double checked in Codespace and the result is similar, instead of triplicating the log output, it's duplicated now (and without the setup_logger() in start_directory_watcher it runs correctly), super weird. I wonder how it's possible that on Mac you don't get the log outputs without the duplicated logging setup.
It's being called already in main() which should be enough for the logs to show up.
Here the screens

I'll check on codespaces once you update the code for my review. I wouldn't be surprised if there's a confounding variable in my local setup that's preventing duplicates logs.

sungchun12 · 2023-04-02T01:17:05Z

The preview limit isn't working for me because from fst.config_defaults import CONFIG on L20 is read by the query_handler.py before it can be reset with the --preview-limit flag. I recommend following some general guidance from GPT4 below that'll point you in the right direction.

sungchun12

See my earlier feedback to refactor this and then I'll review it again!

Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>

sungchun12 · 2023-04-12T18:24:17Z

I'm going to hold off on merging this until 4/19 because I want to present a couple wild features to my company on 4/18. I'll give you a shoutout for all your help!

xtomflo · 2023-04-14T14:54:38Z

sounds good to me! thanks a lot for the shoutout :) I'm traveling at the moment so out of the working routine but will be back next week to make some more progress. good luck with the presentation!

xtomflo · 2023-04-14T15:44:03Z

The preview limit isn't working for me because from fst.config_defaults import CONFIG on L20 is read by the query_handler.py before it can be reset with the --preview-limit flag. I recommend following some general guidance from GPT4 below that'll point you in the right direction.

This is strange as well. It's working on WSL and it's working in Codespaces as well as expected. I'm still updating to your suggestion, it's indeed a bit cleaner

refactor(query_handler.py): remove unused imports and variables, reformat code

sungchun12 · 2023-04-19T16:52:50Z

@@ -22,7 +24,6 @@ def start_streamlit() -> None:
    subprocess.run(["streamlit", "run", streamlit_app_path])

 def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:


I'll check on codespaces once you update the code for my review. I wouldn't be surprised if there's a confounding variable in my local setup that's preventing duplicates logs.

- Add `rows_preview_limit` parameter to `start_directory_watcher` function in `main.py` - Add `rows_preview_limit` parameter to `DynamicQueryHandler` class in `query_handler.py` - Pass `rows_preview_limit` parameter to `handle_query` function in `query_handler.py` - Use `rows_preview_limit` parameter instead of `preview_limit_rows` variable in `handle_query` function in `query_handler.py`

xtomflo · 2023-04-26T09:28:03Z

Added a bunch of changes and should be ready for review.
The duplicated log outputs should be resolved by a check in setup_logger() for an already existing handler.
rows_preview_limit is passed through to the query handler without setting config variable

sungchun12 · 2023-04-26T16:33:46Z

Lovely! I'll take a look this week and add comments!

sungchun12 · 2023-05-01T21:51:56Z

Can you show me a screenshot of this working?

I can't get it to work at all with or without the preview flag: fst start fst start --preview-limit 10.

Please resolve the conflicts of the files above listed by Github!

sungchun12 · 2023-05-01T21:52:25Z


 @lru_cache(maxsize=128)
-def execute_query(query: str, db_file: str) -> Tuple[List[Tuple[Any]], List[str]]:
+def execute_query(query: str, db_file: str,  limit:int = 5) -> Tuple[List[Tuple[Any]], List[str]]:


I like this

sungchun12 added 30 commits March 23, 2023 10:01

sketches

880225d

save this draft

620382a

run on save example

ad16bed

new query handler

0d9a1b4

working draft

5d05d4e

great working draft

1ce0008

default selection

9691783

dynamic metrics

55218c0

fix duckdb storage error

a8548ea

toggle

51220f0

toggle button

173bf0b

iteration v1

a28bfe5

iteration v2

37ebb85

overlay chart working

23f4f16

fixed warnings

f668d38

clean up legend and naming

281ab12

reorder selections new to old

24620cd

update naming

4078fcb

rename file

80bfdd6

v1 of unified fst start

1cba72e

working unified fst start

c62ae84

better error handling

b8a9d01

add new basepath name

b5fc2da

trim the fat

df22d86

color code the bars

300b7ca

color coded bars

81b1e18

save space

7f4cf41

add store failures flag

05b76bf

rename folder

08339cd

add compiled path

336910c

xtomflo added 4 commits March 31, 2023 18:10

Add cli command prompt and default

605a7ae

adding limit var to the preview setting

fe9dcad

Fix the setting, cleaner implementation

71ba56f

refactor(main.py): remove redundant logger instantiation and setup_lo…

c053c3a

…gger call in start_directory_watcher function feat(main.py): add logger instantiation in main function

xtomflo linked an issue Apr 1, 2023 that may be closed by this pull request

set the preview limit row setting: ex: 5 rows, 10 rows, 100, etc. any value that's an integer #14

Open

xtomflo requested a review from sungchun12 April 1, 2023 23:16

sungchun12 reviewed Apr 2, 2023

View reviewed changes

Comment thread fst/main.py Outdated

sungchun12 reviewed Apr 2, 2023

View reviewed changes

Comment thread fst/main.py Outdated

sungchun12 requested changes Apr 2, 2023

View reviewed changes

xtomflo and others added 2 commits April 11, 2023 13:59

Update fst/main.py

038d0c1

Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>

Update fst/main.py

9cf82af

Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>

remove config setting

1914244

xtomflo added 2 commits April 20, 2023 12:58

style(main.py): fix indentation and remove unused imports

b5701a7

refactor(query_handler.py): remove unused imports and variables, reformat code

refactor(logger.py): prevent adding duplicate handlers to logger

6cb5ec3

sungchun12 requested changes Apr 24, 2023

View reviewed changes

xtomflo added 2 commits April 25, 2023 23:17

refactor(config_defaults.py): remove unused CONFIG variable

1971ed3

xtomflo requested a review from sungchun12 April 26, 2023 09:28

sungchun12 reviewed May 1, 2023

View reviewed changes

xtomflo added 2 commits May 5, 2023 12:37

Merge branch 'main' into feature/set-preview-limit

9e8a773

Adding fixes for M1

0d822e8

		@@ -22,7 +24,6 @@ def start_streamlit() -> None:
		subprocess.run(["streamlit", "run", streamlit_app_path])

		def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:

	def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:
	def start_directory_watcher(path: str, log_queue: multiprocessing.Queue) -> None:
	setup_logger(log_queue)

Conversation

xtomflo commented Apr 1, 2023

Uh oh!

sungchun12 commented Apr 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sungchun12 commented Apr 2, 2023

Uh oh!

sungchun12 left a comment

Choose a reason for hiding this comment

Uh oh!

sungchun12 commented Apr 12, 2023

Uh oh!

xtomflo commented Apr 14, 2023

Uh oh!

xtomflo commented Apr 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtomflo commented Apr 26, 2023

Uh oh!

sungchun12 commented Apr 26, 2023

Uh oh!

sungchun12 commented May 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants