search

kaizen-ai / kaizenflow

KaizenFlow is a framework for Bayesian reasoning and AI/ML stream computing
GNU General Public License v3.0
109 stars 76 forks source link

Make `_to_multiline_cmd()` public #259

Closed DanilYachmenev closed 1 year ago

DanilYachmenev commented 1 year ago

Posted originally by @PomazkinG at https://github.com/cryptokaizen/cmamp/issues/755

Make _to_multiline_cmd() from helpers/lib_tasks_utils.py a public function:

Assigning @samarth9008 to re-distribute

gpsaggese commented 1 year ago

From Christy

But the name of the function given here is a bit different. When I went into the mentioned folder helpers/lib_tasks_utils.py i didn;t find a function with name _to_multiline_cmd() instead i found one with _to_multi_line_cmd(). Can I proceed with making changes on the later one? I have sent a comment yesterday asking this question, didn't get any response yet.

Could you please clarify this for me?

gpsaggese commented 1 year ago

Yes, the name of the function was misspelled.

It's ok to ask for confirmation but the evidence is strongly pointing in that direction, in fact 1) there is only one function in the code base called _to_multil_line_cmd() and not _to_multilline_cmd() 2) the function is actually in helpers/lib_tasks_utils.py as requested 3) It is used as a public function although is declared private

christyanthony commented 1 year ago

Thank You for the confirmation

christyanthony commented 1 year ago

Hello @samarth9008 @DanilYachmenev ,

Just for a confirmation, I found the function in all these packages

image

I assume that the change needs to be reflected in all these packages, Is my understanding correct? or the change has to reflected only in the given package?

samarth9008 commented 1 year ago

I think it would be all the places. Wdyt @DanilYachmenev?

If you are a windows user, please go through this doc for better usability.

DanilYachmenev commented 1 year ago

I assume that the change needs to be reflected in all these packages, Is my understanding correct? or the change has to reflected only in the given package?

yes, otherwise, it will break all the other files In general, if you rename a function, make sure you update all its calls

If you have no access to some files, its ok The most important thing is to make a script using replace_text.py - this way we can run it and do renaming in all the files

christyanthony commented 1 year ago

I am running the .sh file I created, but it is giving the below error.

image

I already have python installed in the system. Do i have to do anything like adding path(I have tried doing this and still got the same error).

Below is the .sh file

image
samarth9008 commented 1 year ago

Use linux if you are a windows user. I have pointed you to the doc for the same.

gpsaggese commented 1 year ago

@christyanthony pls switch to Linux. We don't support Windows

christyanthony commented 1 year ago

Ok @gpsaggese , @samarth9008 , will setup the linux and see the work

christyanthony commented 1 year ago

@yiyunlei I am facing this error while running the script I created to replace the texts. image

Below is the script I created to replace texts image

yiyunlei commented 1 year ago

@christyanthony Please paste your script code here so that it's convenient for me to use.

samarth9008 commented 1 year ago

@yiyunlei Instead, you can switch to her branch and execute it.

christyanthony commented 1 year ago

Shall i do a pull request then?

yiyunlei commented 1 year ago

@samarth9008 Yeah, it's a good idea, but she didn't push it. @christyanthony Yes, please push it to a new branch.

yiyunlei commented 1 year ago

Shall i do a pull request then?

Don't pull request for now, just push it to a new branch, so I can use your code.

samarth9008 commented 1 year ago

image

samarth9008 commented 1 year ago

On the right side.

christyanthony commented 1 year ago

i just did

DanilYachmenev commented 1 year ago

@christyanthony you can do a pull request with the changes you made @yiyunlei you can just check in to the PR branch when it is pushed and do changes there But I'd rather recommend you to not commit any changes to other branches but leave comments on the PR

yiyunlei commented 1 year ago

@christyanthony I changed some code and tried several times, and it can run successfully now: My code:

#!/bin/bash -xe

dir_names="amp/dev_scripts/cleanup_scripts dev_scripts/cleanup_scripts"

replace_text.py \
   --old "_to_multi_line_cmd" \
   --new "to_multi_line_cmd" \
   --exclude_dirs "$dir_names"

Output:

image

Use VScode to double-check:

image

So all _to_multi_line_cmd were changed to to_multi_line_cmd, except the rename script.

The key points:

yiyunlei commented 1 year ago

@christyanthony I didn't commit my code, so you can use my code and try it, then commit by yourself.

yiyunlei commented 1 year ago

I just found this script can run both inside the docker and outside the docker. Is this reason why there are two excluded dirs?

yiyunlei commented 1 year ago

@christyanthony Please paste your error logs (not screenshots) here if you are still encountering bugs.

christyanthony commented 1 year ago

This is the error I got when running inside '/app/ with ./dev_scripts/cleanup_scripts/Rename_to_multi_line_cmd.sh

I have no name!@769f33f5b032:/app$ ./dev_scripts/cleanup_scripts/Rename_to_multi_line_cmd.sh

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Traceback (most recent call last): File "/app/dev_scripts/replace_text.py", line 762, in _main(_parse()) File "/app/dev_scripts/replace_text.py", line 656, in _main file_names = _get_all_files(dirs, exts) File "/app/dev_scripts/replace_text.py", line 93, in _get_all_files file_namestmp = hio.listdir( File "/app/helpers/hio.py", line 82, in listdir , output = hsystem.system_to_string(cmd) File "/app/helpers/hsystem.py", line 343, in system_to_string rc, output = _system( File "/app/helpers/hsystem.py", line 276, in _system raise RuntimeError( RuntimeError: cmd='(find . -name ".py" -not -path "/.git/*") 2>&1' failed with rc='1' truncated output= ./im_v2/binance/notebooks/CmTask4044_Binance_native_data_source.py ./im_v2/binance/data/init.py ./im_v2/binance/data/extract/test/test_binance_extractor.py ./im_v2/binance/data/extract/extractor.py ./im_v2/binance/data/extract/init.py ./im_v2/binance/init.py ./im_v2/devops/init.py ./im_v2/test/test_im_lib_tasks.py ./im_v2/cdd/data/init.py ./im_v2/cdd/data/extract/init.py ./im_v2/cdd/data/extract/download_historical.py ./im_v2/cdd/init.py ./im_v2/airflow/init.py ./im_v2/airflow/clear_stuck_airflow_tasks.py ./im_v2/airflow/dags/preprod.download_periodic_daily_ohlcv_data_fargate.py ./im_v2/airflow/dags/preprod.download_resample_periodic_daily_bid_ask_data_fargate.py ./im_v2/airflow/dags/preprod.download_resample_periodic_1min_data_websocket_fargate.py ./im_v2/airflow/dags/preprod.data_qa_periodic_daily_fargate.py ./im_v2/airflow/dags/init.py ./im_v2/airflow/dags/preprod.download_periodic_1min_ohlcv_data_rest_fargate.py ./im_v2/airflow/dags/preprod.daily_paper_system_run_reconcile_fargate.py ./im_v2/airflow/dags/preprod.data_qa_periodic_10min_fargate.py ./im_v2/airflow/dags/preprod.download_periodic_daily_trades_data_fargate.py ./im_v2/airflow/dags/preprod.postgres_data_archival_to_s3_fargate.py ./im_v2/airflow/dags/airflow_utils/init.py ./im_v2/airflow/dags/airflow_utils/telegram/init.py ./im_v2/airflow/dags/airflow_utils/telegram/operator.py ./im_v2/init.py ./im_v2/mock1/init.py ./im_v2/mock1/universe/test/init.py

gpsaggese commented 1 year ago

IIRC the replace script runs outside the docker container since it doesn't need all the deps. It excludes some dirs to avoid modifying itself I thought we had some documentation about how to run this script. @rheenina do you recall?

Not sure what's the problem that it's making this task complicated. It should be just a one-line invocation to the script.

christyanthony commented 1 year ago

It would be really helpful if there is a documentation on it.

yiyunlei commented 1 year ago

Two suggestions:

  1. Try to run the same script outside of Docker and see if it works, like below, and it also can run successfully on my side.

    image

  2. Check the error logs again if it still doesn't work. Below is the most important part of the error logs.

Traceback (most recent call last):
File "/app/dev_scripts/replace_text.py", line 762, in
_main(_parse())
File "/app/dev_scripts/replace_text.py", line 656, in _main
file_names = _get_all_files(dirs, exts)
File "/app/dev_scripts/replace_text.py", line 93, in _get_all_files
file_names_tmp = hio.listdir(
File "/app/helpers/hio.py", line 82, in listdir
_, output = hsystem.system_to_string(cmd)
File "/app/helpers/hsystem.py", line 343, in system_to_string
rc, output = _system(
File "/app/helpers/hsystem.py", line 276, in _system
raise RuntimeError(
RuntimeError: cmd='(find . -name ".py" -not -path "/.git/*") 2>&1' failed with rc='1'
truncated output=

The most recent call is

File "/app/helpers/hsystem.py", line 276, in _system
raise RuntimeError(
RuntimeError: cmd='(find . -name ".py" -not -path "/.git/*") 2>&1' failed with rc='1'

It means that the error occurs when it wants to execute the command line 

(find . -name ".py" -not -path "/.git/*") 2>&1

I hope this helps you with debugging.

Since I don't get this bug, and I'm unfamiliar with file "/app/helpers/hsystem.py", I'm not sure why this bug occurs.

PomazkinG commented 1 year ago

hsystem just executes the cmd. can you try just execute it manually and see what happens? simply run:

> find . -name ".py" -not -path "/.git/*"
christyanthony commented 1 year ago

Sure will try and let you guys know.

samarth9008 commented 1 year ago

@christyanthony Even I am using VM and the script works totally fine inside and outside docker. There can be some error related to your installation.

PomazkinG commented 1 year ago

alternatively if it works outside Docker for you -> no need to try running inside Docker, i.e. run from wherever it works for you

yiyunlei commented 1 year ago

If there are still issues, maybe try reinstalling and setting up your environment.

christyanthony commented 1 year ago

I feel like I have to re-install and set up the environment. But before I do that:

I tried all the above mentioned methods. I ran > find . -name ".py" -not -path "/.git/*" in the container and then again tried running my script after this. I faced same error

I also tried running my script outside the docker, it gave me the error related to file not found. Does this have have anything to do with setting up the environment. This is the error:

christy@christy-virtual-machine:~/src/sorrentum1$ ./dev_scripts/cleanup_scripts/Rename_to_multi_line_cmd.sh

But the said file is available and also python is installed in the system.

yiyunlei commented 1 year ago

Run this to activate your environment first, then run the rename script. > source dev_scripts/setenv_amp.sh

christyanthony commented 1 year ago

I did that too got the same error like when I ran in the container

rheenina commented 1 year ago

I thought we had some documentation about how to run this script. @rheenina do you recall?

I'm not sure we have one, but the script has a pretty good doc string dev_scripts/replace_text.py

christyanthony commented 1 year ago

Good News Guys! It works!!! I set up the whole thing again and tried running the newly created script. It worked fine. image Thank You each and everyone for sitting with me on this issue and helping me out. :)

yiyunlei commented 1 year ago

@christyanthony good job!

DanilYachmenev commented 1 year ago

Done