[Good First Issue julep-ai#555] Added QUESTION and FEEDBACK comments …

…across the codebase

agents-api/agents_api/activities/embed_docs.py

@@ -8,8 +8,18 @@
 
 
 @beartype
+
+# FEEDBACK[@Bhabuk10]: Great use of the `beartype` decorator to ensure type checking
+# at runtime. However, if the project allows for stricter static type checking, consider using 
+# Python's `mypy` as well. It can complement `beartype` and catch more issues during development.
+
 async def embed_docs(payload: EmbedDocsPayload, cozo_client=None) -> None:
     indices, snippets = list(zip(*enumerate(payload.content)))
+
+    # QUESTION[@Bhabuk10]: What is the rationale for using `enumerate` here?
+    # Is the index being used later on, or is it necessary in this context? 
+    # If indices are always needed for snippets, a comment explaining their use would help clarify.
+
     embed_instruction: str = payload.embed_instruction or ""
     title: str = payload.title or ""
 
@@ -39,3 +49,10 @@ async def mock_embed_docs(payload: EmbedDocsPayload, cozo_client=None) -> None:
 embed_docs = activity.defn(name="embed_docs")(
     embed_docs if not testing else mock_embed_docs
 )
+# QUESTION[@Bhabuk10]: What is the purpose of this conditional? 
+# Should the code switch between `embed_docs` and `mock_embed_docs` based purely on the `testing` variable?
+# Clarification on the conditions under which this switch happens would be helpful.
+
+# FEEDBACK[@Bhabuk10]: Consider improving the clarity of the conditional that switches 
+# between `embed_docs` and `mock_embed_docs`. It may be better to centralize the testing condition 
+# logic so that it's easier to extend or modify in the future.

agents-api/agents_api/activities/excecute_api_call.py

@@ -21,6 +21,9 @@ class RequestArgs(TypedDict):
     cookies: Optional[dict[str, str]]
     params: Optional[Union[str, dict[str, Any]]]
 
+    # QUESTION[@Bhabuk10]: Why is `json_` named with an underscore? Is it to avoid conflicting with 
+    # a Python keyword or another variable? Adding a comment to explain this decision would 
+    # improve clarity for other developers.
 
 @beartype
 async def execute_api_call(
@@ -36,13 +39,19 @@ async def execute_api_call(
                 follow_redirects=api_call.follow_redirects,
                 **request_args,
             )
+            # FEEDBACK[@Bhabuk10]: Consider adding timeout handling for the `httpx` client. 
+            # Timeouts are essential for making the application resilient to network issues. 
+            # You could add a timeout parameter to `AsyncClient` for better control over the request.
 
         response_dict = {
             "status_code": response.status_code,
             "headers": dict(response.headers),
             "content": response.content,
             "json": response.json(),
         }
+        # FEEDBACK[@Bhabuk10]: While this is a comprehensive response, it would be a good idea to add 
+        # exception handling around `response.json()` to handle cases where the response body isn't 
+        # valid JSON, which could prevent unwanted crashes in those cases.
 
         return response_dict
 
@@ -51,10 +60,16 @@ async def execute_api_call(
             activity.logger.error(f"Error in execute_api_call: {e}")
 
         raise
-
+        # QUESTION[@Bhabuk10]: Why is `BaseException` being caught here instead of more specific 
+        # exceptions like `httpx.HTTPError`? Catching broad exceptions can make debugging harder. 
+        # Consider catching more specific exceptions to handle different cases effectively.
 
 mock_execute_api_call = execute_api_call
 
 execute_api_call = activity.defn(name="execute_api_call")(
     execute_api_call if not testing else mock_execute_api_call
 )
+
+# FEEDBACK[@Bhabuk10]: It's great that testing is considered here with `mock_execute_api_call`. 
+# It might be useful to provide more context on how `mock_execute_api_call` differs from 
+# the actual `execute_api_call`, especially for new developers or contributors unfamiliar with this pattern.

agents-api/agents_api/activities/execute_integration.py

@@ -30,6 +30,11 @@ async def execute_integration(
         developer_id=developer_id, agent_id=agent_id, task_id=task_id, arg_type="setup"
     )
 
+    # FEEDBACK[@Bhabuk10]: The use of the `|` operator to merge dictionaries is a great choice 
+    # for readability. However, be mindful of Python version compatibility since `|` is only 
+    # supported in Python 3.9 and above. If backward compatibility is a concern, you may want to 
+    # consider an alternative merging strategy.
+
     arguments = (
         merged_tool_args.get(tool_name, {}) | (integration.arguments or {}) | arguments
     )
@@ -40,6 +45,10 @@ async def execute_integration(
         if integration.provider == "dummy":
             return arguments
 
+        # FEEDBACK[@Bhabuk10]: It's unclear from the code what the "dummy" provider is 
+        # expected to do. Consider adding a comment explaining this scenario or, alternatively, 
+        # refactor it into a separate function to improve readability and isolate this logic.
+
         return await integrations.run_integration_service(
             provider=integration.provider,
             setup=setup,
@@ -52,10 +61,18 @@ async def execute_integration(
             activity.logger.error(f"Error in execute_integration: {e}")
 
         raise
-
+        # QUESTION[@Bhabuk10]: Why is `BaseException` being caught instead of more specific 
+        # exceptions (e.g., `KeyError`, `TypeError`, or integration-specific exceptions)? 
+        # It might be useful to catch more granular exceptions to allow for better error handling 
+        # and debugging.
 
 mock_execute_integration = execute_integration
 
 execute_integration = activity.defn(name="execute_integration")(
     execute_integration if not testing else mock_execute_integration
 )
+
+# FEEDBACK[@Bhabuk10]: This structure to handle testing with `mock_execute_integration` is a 
+# solid pattern. However, it may be beneficial to document how `mock_execute_integration` 
+# is expected to behave compared to the actual `execute_integration`. This could help future 
+# contributors understand the purpose and limitations of the mock.

agents-api/agents_api/workflows/demo.py

@@ -7,14 +7,27 @@
 with workflow.unsafe.imports_passed_through():
     from ..activities.demo import demo_activity
 
+# FEEDBACK[@Bhabuk10]: It might be useful to explain in a comment why `workflow.unsafe.imports_passed_through()` 
+# is necessary here. It seems to allow importing modules in a context where this is normally restricted, but 
+# clarifying its intent would help new contributors understand its use and potential risks.
 
 @workflow.defn
 class DemoWorkflow:
     @workflow.run
     async def run(self, a: int, b: int) -> int:
+        # QUESTION[@Bhabuk10]: Could you clarify why `a` and `b` are being passed as integers to this workflow? 
+        # Are there other types that this workflow is expected to handle? If so, is type-checking in place elsewhere?
+
         return await workflow.execute_activity(
             demo_activity,
             args=[a, b],
             start_to_close_timeout=timedelta(seconds=30),
             retry_policy=DEFAULT_RETRY_POLICY,
         )
+        # FEEDBACK[@Bhabuk10]: Consider adding a comment explaining the choice of a 30-second timeout. 
+        # It would be helpful to understand why this specific timeout was selected and whether it is subject to change 
+        # depending on the activity's complexity or resource usage.
+
+        # FEEDBACK[@Bhabuk10]: The use of `DEFAULT_RETRY_POLICY` is good, but consider documenting what 
+        # this default policy is and why it is suitable for this activity. Including some context on when 
+        # to customize the retry policy (e.g., for more critical activities) might be useful for future contributors.

agents-api/agents_api/workflows/embed_docs.py

@@ -21,3 +21,9 @@ async def run(self, embed_payload: EmbedDocsPayload) -> None:
             schedule_to_close_timeout=timedelta(seconds=600),
             retry_policy=DEFAULT_RETRY_POLICY,
         )
+        # FEEDBACK[@Bhabuk10]: The timeout of 600 seconds (10 minutes) seems quite large for a document embedding task. 
+        # Consider adding a comment explaining why this particular timeout was chosen, or if it's meant to accommodate 
+        # a wide range of document sizes. Additionally, adding a configurable timeout could give more flexibility 
+        # depending on the nature of the document being embedded.
+
+

agents-api/agents_api/workflows/mem_mgmt.py

@@ -10,6 +10,10 @@
     from ..autogen.openapi_model import InputChatMLMessage
 
 
+# FEEDBACK[@Bhabuk10]: A module-level docstring would be helpful here to explain the purpose of this workflow.
+# Specifically, describe how memory management is handled with the provided inputs and clarify any assumptions 
+# regarding `InputChatMLMessage` and memory types.
+
 @workflow.defn
 class MemMgmtWorkflow:
     @workflow.run
@@ -19,6 +23,11 @@ async def run(
         session_id: str,
         previous_memories: list[str],
     ) -> None:
+
+        # QUESTION[@Bhabuk10]: Is there validation elsewhere to ensure that `dialog` is well-formed 
+        # or that the `session_id` and `previous_memories` are valid? If not, it might be worth adding 
+        # input validation to catch issues early and prevent activity failure.
+
         return await workflow.execute_activity(
             mem_mgmt,
             [dialog, session_id, previous_memories],

agents-api/tests/test_activities.py

@@ -27,6 +27,10 @@ async def _(
     content = ["content 1"]
     include_title = True
 
+    # QUESTION[@Bhabuk10]: Why is the `include_title` flag set to True in this test case? 
+    # Is there a scenario where embedding behavior would change based on this flag's value? 
+    # It may be useful to add a test case that explicitly verifies this.
+
     await embed_docs(
         EmbedDocsPayload(
             developer_id=developer_id,
@@ -39,6 +43,9 @@ async def _(
         cozo_client,
     )
 
+    # FEEDBACK[@Bhabuk10]: It would be beneficial to include assertions here that verify the 
+    # expected outcome after calling `embed_docs`. This could be checking that the document was 
+    # embedded correctly or validating the result in some way to ensure that the function works as intended.
 
 @test("activity: call demo workflow via temporal client")
 async def _():
@@ -54,4 +61,12 @@ async def _():
         )
 
         assert result == 3
+
+        # FEEDBACK[@Bhabuk10]: Consider adding a comment explaining why the expected result is 3. 
+        # Including a brief description of how `DemoWorkflow` works can make the test more understandable to others.
+
         mock_get_client.assert_called_once()
+
+        # FEEDBACK[@Bhabuk10]: Good practice in verifying that the temporal client was called once. 
+        # You may also want to test edge cases like invalid input arguments or failure scenarios to 
+        # increase the coverage of the test suite.

agents-api/tests/test_agent_queries.py

@@ -1,4 +1,3 @@
-# Tests for agent queries
 from uuid import uuid4
 
 from ward import raises, test
@@ -33,6 +32,7 @@ def _(client=cozo_client, developer_id=test_developer_id):
         client=client,
     )
 
+    # FEEDBACK[@Bhabuk10]: Consider adding an assertion after the agent creation to verify that the agent was actually created and persisted in the database. This would improve test coverage.
 
 @test("model: create agent with instructions")
 def _(client=cozo_client, developer_id=test_developer_id):
@@ -47,6 +47,7 @@ def _(client=cozo_client, developer_id=test_developer_id):
         client=client,
     )
 
+    # FEEDBACK[@Bhabuk10]: As with the previous test, it would be beneficial to add an assertion here to validate that the agent with instructions has been created correctly. Testing the `instructions` field would ensure it's properly handled.
 
 @test("model: create or update agent")
 def _(client=cozo_client, developer_id=test_developer_id):
@@ -62,6 +63,7 @@ def _(client=cozo_client, developer_id=test_developer_id):
         client=client,
     )
 
+    # QUESTION[@Bhabuk10]: What is the expected behavior if an agent with the given `agent_id` already exists? Should it update the existing agent or throw an error? It might be worth testing both cases to ensure that the function works correctly in both scenarios.
 
 @test("model: get agent not exists")
 def _(client=cozo_client, developer_id=test_developer_id):
@@ -70,6 +72,7 @@ def _(client=cozo_client, developer_id=test_developer_id):
     with raises(Exception):
         get_agent(agent_id=agent_id, developer_id=developer_id, client=client)
 
+    # FEEDBACK[@Bhabuk10]: Good use of `raises(Exception)` to test the absence of an agent. It might help to make the exception more specific (if possible) to avoid catching unintended exceptions.
 
 @test("model: get agent exists")
 def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
@@ -78,6 +81,7 @@ def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
     assert result is not None
     assert isinstance(result, Agent)
 
+    # FEEDBACK[@Bhabuk10]: You could also add assertions that check the specific values in the `Agent` object to ensure that the data is being fetched correctly.
 
 @test("model: delete agent")
 def _(client=cozo_client, developer_id=test_developer_id):
@@ -99,6 +103,7 @@ def _(client=cozo_client, developer_id=test_developer_id):
     with raises(Exception):
         get_agent(agent_id=temp_agent.id, developer_id=developer_id, client=client)
 
+    # QUESTION[@Bhabuk10]: What happens if you try to delete an agent that doesn't exist? Should this raise an exception, or is there a different expected behavior? Consider testing that scenario as well.
 
 @test("model: update agent")
 def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
@@ -152,6 +157,7 @@ def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
 
     assert "hello" in agent.metadata
 
+    # QUESTION[@Bhabuk10]: Does patching overwrite or merge existing fields? It might be helpful to add a test case that ensures the correct behavior when patching fields that already exist in the agent.
 
 @test("model: list agents")
 def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
@@ -161,3 +167,4 @@ def _(client=cozo_client, developer_id=test_developer_id, agent=test_agent):
 
     assert isinstance(result, list)
     assert all(isinstance(agent, Agent) for agent in result)
+