KaizenTask768_Add_smoke_test_for_lib_tasks_run_model_experiment_notebooks

[Jd8111997]

Hi @sonaalKant,

I've written a unit test for the run_notebooks method. Could you please review the logic of the function and let me know if I'm heading in the right direction?

Also, I want to mention that this is just a draft PR, so the code hasn't been refactored yet.

While running the pytest, I encountered the following error:

Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.9/unittest/case.py", line 593, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/app/dev_scripts/test/test_run_notebook.py", line 574, in test1
    run_experiment_notebooks.run_notebooks(
  File "/venv/lib/python3.9/site-packages/invoke/tasks.py", line 134, in __call__
    if not isinstance(args[0], Context):
IndexError: tuple index out of range

I am mocking a Context object and passing it to the run_notebooks method. Do you have any insights into why it's failing? Since I couldn't run this on real-world data, could you please help me with this?

[sonaalKant]

This was not the expectation at all, mocking everything destroys the purpose of having a unit test. We want to test the flow, if there are any changes in config or in the function reading the config then the test should be able to catch this.

You only need two unit tests one for broker_only and one for full system:

Steps:

1. Create a temp directory and store the config file (created by you) in that temp directory. (args.json for broker only run and pkl file for full system )
2. Mock _run_notebook function to return nothing

Also, test_run_notebooks.py is not the correct file to place your code. Go through the documentation again https://github.com/cryptokaizen/cmamp/blob/e57bd0e992a82e40f8e48fa977c105bd2b4f45b6/docs/coding/all.write_unit_tests.how_to_guide.md#naming-and-placement-conventions if things are not clear.

[Jd8111997]

@sonaalKant, I updated a PR, could you please review it and let me know if I am heading into the right direction.

Also, What's the use of ctx object in run_notebooks method ? I see we are passing it to the method but not using it. I tried to pass None but I am getting the error and I also tried to mock it but still the same error persists. Let me know if you have any solution in mind.

This is the error I'm getting:

_____________________________________________ Test_run_notebooks.test1 _____________________________________________
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.9/unittest/case.py", line 593, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/app/dev_scripts/test/test_lib_tasks_run_model_experiment_notebooks.py", line 48, in test1
    run_experiment_notebooks.run_notebooks(
  File "/venv/lib/python3.9/site-packages/invoke/tasks.py", line 134, in __call__
    if not isinstance(args[0], Context):
IndexError: tuple index out of range

[samarth9008]

One suggestion from my side would be to shift this PR on cmamp side so Jaydeep can more clarity and it can be tested properly.

Would be nice if Sonaal can point Jaydeep to some similar test case for reference.

If we don't have any, a PP with Jaydeep can make things more clear I believe.

[Jd8111997]

One suggestion from my side would be to shift this PR on cmamp side so Jaydeep can more clarity and it can be tested properly.

Would be nice if Sonaal can point Jaydeep to some similar test case for reference.

If we don't have any, a PP with Jaydeep can make things more clear I believe.

@sonaalKant, this might help.

[sonaalKant]

One suggestion from my side would be to shift this PR on cmamp side so Jaydeep can more clarity and it can be tested properly.

Agree. @Jd8111997 Once you are done can you close this PR here and create a final PR on cmamp directly. https://github.com/cryptokaizen/cmamp/issues/7782

[Jd8111997]

@sonaalKant, there is a bug in the run_notebooks function. We are logging the price_col and table_name variables outside of the if.. else.. statement, but they are only defined for a full_system_run. Therefore, in the broker_only scenario, it throws the following error:

UnboundLocalError: local variable 'price_col' referenced before assignment

These variables are also utilized later in the same function:

master_exec = (
        "amp/oms/notebooks/Master_execution_analysis.ipynb",
        "amp.oms.execution_analysis_configs."
        + f'get_execution_analysis_configs_Cmtask4881("{system_log_dir}", \
            "{bar_duration}", \
            "{universe_version}", \
            "{child_order_execution_freq}", \
            "{price_col}", \
            "{table_name}", \
            test_asset_id={test_asset_id})',
    )

[sonaalKant]

Good catch. Thats exactly why we need unit test to check the flow. I see some changes were made 3 weeks back but this case got missed.

[sonaalKant]

Shifted to cmamp and merged https://github.com/cryptokaizen/cmamp/issues/7782. Closing here