Berack/upo-app-agents
0
0
Fork 0
Code Issues Pull Requests 3 Actions Packages Projects Releases Wiki Activity

WIP: Fix Aggregazione market Product #67

Draft
Simo93-rgb wants to merge 13 commits from 62-aggregazione-market-product-non-corretta into main
pull from: 62-aggregazione-market-product-non-corretta
merge into: Berack:main
Conversation 12 Commits 13 Files Changed 4 +151 -28
Simo93-rgb commented 2025-10-30 15:40:02 +01:00 (Migrated from github.com)
Copy Link

Potrebbero esserci ancora delle cose da rivedere ma per ora mi sento di dire che è una roba sicura

Potrebbero esserci ancora delle cose da rivedere ma per ora mi sento di dire che è una roba sicura
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 15:41:21 +01:00
src/app/agents/prompts/team_market.md
Simo93-rgb (Migrated from github.com) commented 2025-10-30 15:41:21 +01:00
Copy Link

Questo sarebbe dovuto andare nell'issue opportuno ma può anche stare qui. Verrà perfezionato nel branch 64

Questo sarebbe dovuto andare nell'issue opportuno ma può anche stare qui. Verrà perfezionato nel branch 64
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 15:42:32 +01:00
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
Simo93-rgb (Migrated from github.com) commented 2025-10-30 15:42:32 +01:00
Copy Link

Aggiunto metodo di aggregazione singola, serve sia per un metodo GET del tool market sia per snellire la logica del secondo metodo di aggregazione quello di aggregazione multipla.

Aggiunto metodo di aggregazione singola, serve sia per un metodo GET del tool market sia per snellire la logica del secondo metodo di aggregazione quello di aggregazione multipla.
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-10-30 15:43:56 +01:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy Link

Pull Request Overview

This PR refactors the market data aggregation system by splitting the aggregation logic into two distinct methods (aggregate_single_asset and aggregate_multi_assets) and adds comprehensive test coverage for various edge cases. The changes improve API clarity and fix volume calculation bugs.

  • Renamed ProductInfo.aggregate() to aggregate_multi_assets() and introduced aggregate_single_asset() for better separation of concerns
  • Added provider field to ProductInfo model and populated it across all market data extractors
  • Fixed volume calculation in tests (changed from average to total volume)
  • Enhanced documentation with detailed tool descriptions and usage guidelines

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
src/app/api/core/markets.py Split aggregation into aggregate_single_asset() and aggregate_multi_assets() methods; fixed volume calculation logic
src/app/api/tools/market_tool.py Added get_product_aggregated() method and updated get_products_aggregated() to use new aggregation approach
src/app/api/markets/*.py Added provider field assignment in all market data extractors (Binance, Coinbase, CryptoCompare, YFinance)
tests/utils/test_market_aggregator.py Updated test suite to use new methods, fixed volume assertions, added 14 new test cases for edge cases
src/app/api/tools/symbols_tool.py Enhanced documentation, improved error handling, updated search logic to include both name and symbol
src/app/agents/prompts/*.md Expanded tool documentation with detailed descriptions and selection strategies
src/app/agents/plan_memory_tool.py Added docstring for Task TypedDict and corrected toolkit name
configs.yaml.example Removed obsolete gpt-oss:latest model entry

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull Request Overview This PR refactors the market data aggregation system by splitting the aggregation logic into two distinct methods (`aggregate_single_asset` and `aggregate_multi_assets`) and adds comprehensive test coverage for various edge cases. The changes improve API clarity and fix volume calculation bugs. - Renamed `ProductInfo.aggregate()` to `aggregate_multi_assets()` and introduced `aggregate_single_asset()` for better separation of concerns - Added `provider` field to `ProductInfo` model and populated it across all market data extractors - Fixed volume calculation in tests (changed from average to total volume) - Enhanced documentation with detailed tool descriptions and usage guidelines ### Reviewed Changes Copilot reviewed 12 out of 12 changed files in this pull request and generated 15 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | src/app/api/core/markets.py | Split aggregation into `aggregate_single_asset()` and `aggregate_multi_assets()` methods; fixed volume calculation logic | | src/app/api/tools/market_tool.py | Added `get_product_aggregated()` method and updated `get_products_aggregated()` to use new aggregation approach | | src/app/api/markets/*.py | Added `provider` field assignment in all market data extractors (Binance, Coinbase, CryptoCompare, YFinance) | | tests/utils/test_market_aggregator.py | Updated test suite to use new methods, fixed volume assertions, added 14 new test cases for edge cases | | src/app/api/tools/symbols_tool.py | Enhanced documentation, improved error handling, updated search logic to include both name and symbol | | src/app/agents/prompts/*.md | Expanded tool documentation with detailed descriptions and selection strategies | | src/app/agents/plan_memory_tool.py | Added docstring for `Task` TypedDict and corrected toolkit name | | configs.yaml.example | Removed obsolete `gpt-oss:latest` model entry | </details> --- 💡 <a href="/Berack96/upo-appAI/new/main/.github?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:51 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Building map symbol -> list of ProductInfo (from all providers)'.

        # Building map symbol -> list of ProductInfo (from all providers)
Italian comment in English codebase. Should be translated to 'Building map symbol -> list of ProductInfo (from all providers)'. ```suggestion # Building map symbol -> list of ProductInfo (from all providers) ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:52 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Ensure that the provider is set'.

                # Ensure that the provider is set
Italian comment in English codebase. Should be translated to 'Ensure that the provider is set'. ```suggestion # Ensure that the provider is set ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:52 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Aggregation for each symbol using aggregate_single_asset'.

        # Aggregation for each symbol using aggregate_single_asset
Italian comment in English codebase. Should be translated to 'Aggregation for each symbol using aggregate_single_asset'. ```suggestion # Aggregation for each symbol using aggregate_single_asset ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:52 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Use aggregate_single_asset to aggregate each symbol'.

                # Use aggregate_single_asset to aggregate each symbol
Italian comment in English codebase. Should be translated to 'Use aggregate_single_asset to aggregate each symbol'. ```suggestion # Use aggregate_single_asset to aggregate each symbol ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:53 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'If aggregate_single_asset fails (e.g., no USD when currencies differ), skip'.

                # If aggregate_single_asset fails (e.g., no USD when currencies differ), skip
Italian comment in English codebase. Should be translated to 'If aggregate_single_asset fails (e.g., no USD when currencies differ), skip'. ```suggestion # If aggregate_single_asset fails (e.g., no USD when currencies differ), skip ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:53 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Currency check: if not all the same, filter only USD'.

        # Currency check: if not all the same, filter only USD
Italian comment in English codebase. Should be translated to 'Currency check: if not all the same, filter only USD'. ```suggestion # Currency check: if not all the same, filter only USD ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:53 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Different currencies: filter only USD'.

            # Different currencies: filter only USD
Italian comment in English codebase. Should be translated to 'Different currencies: filter only USD'. ```suggestion # Different currencies: filter only USD ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:54 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Aggregation for each exchange'.

        # Aggregation for each exchange
Italian comment in English codebase. Should be translated to 'Aggregation for each exchange'. ```suggestion # Aggregation for each exchange ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:54 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Collect providers that have provided data'.

        # Collect providers that have provided data
Italian comment in English codebase. Should be translated to 'Collect providers that have provided data'. ```suggestion # Collect providers that have provided data ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:54 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Average volume calculation'.

        # Average volume calculation
Italian comment in English codebase. Should be translated to 'Average volume calculation'. ```suggestion # Average volume calculation ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:54 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Volume-weighted price calculation (VWAP - Volume Weighted Average Price)'.

        # Volume-weighted price calculation (VWAP - Volume Weighted Average Price)
Italian comment in English codebase. Should be translated to 'Volume-weighted price calculation (VWAP - Volume Weighted Average Price)'. ```suggestion # Volume-weighted price calculation (VWAP - Volume Weighted Average Price) ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:55 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'If there is no volume, we do a simple average of prices'.

            # If there is no volume, we do a simple average of prices
Italian comment in English codebase. Should be translated to 'If there is no volume, we do a simple average of prices'. ```suggestion # If there is no volume, we do a simple average of prices ```
src/app/api/tools/market_tool.py
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:51 +01:00
Copy Link

The type annotation indicates dict[str, list[ProductInfo]] but try_call_all() returns dict[str, ProductInfo] (mapping provider name to ProductInfo). This creates a type mismatch where all_products[asset] is assigned a dict instead of a list. The variable should be typed as dict[str, dict[str, ProductInfo]] or the dict should be converted to a list.

            all_products[asset] = list(self.handler.try_call_all(lambda w: w.get_product(asset)).values())
The type annotation indicates `dict[str, list[ProductInfo]]` but `try_call_all()` returns `dict[str, ProductInfo]` (mapping provider name to ProductInfo). This creates a type mismatch where `all_products[asset]` is assigned a dict instead of a list. The variable should be typed as `dict[str, dict[str, ProductInfo]]` or the dict should be converted to a list. ```suggestion all_products[asset] = list(self.handler.try_call_all(lambda w: w.get_product(asset)).values()) ```
src/app/api/tools/symbols_tool.py
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:55 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Search both in name and symbol, ignoring case'.

            # Search both in name and symbol, ignoring case
Italian comment in English codebase. Should be translated to 'Search both in name and symbol, ignoring case'. ```suggestion # Search both in name and symbol, ignoring case ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-10-30 15:43:56 +01:00
Copy Link

Italian comment in English codebase. Should be translated to 'Convert the result to a list of tuples'.

            # Convert the result to a list of tuples
Italian comment in English codebase. Should be translated to 'Convert the result to a list of tuples'. ```suggestion # Convert the result to a list of tuples ```
Berack96 (Migrated from github.com) reviewed 2025-10-30 20:12:05 +01:00
configs.yaml.example
Berack96 (Migrated from github.com) commented 2025-10-30 20:09:10 +01:00
Copy Link

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR
src/app/agents/prompts/team_leader.md
Berack96 (Migrated from github.com) commented 2025-10-30 20:09:00 +01:00
Copy Link

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR
src/app/agents/prompts/team_market.md
Berack96 (Migrated from github.com) commented 2025-10-30 20:09:04 +01:00
Copy Link

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR

Questo file, insieme ad altri, non dovrebbe essere committato in questo branch, dato che causa errori di merge con il main e non interessa nella PR
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
Berack96 (Migrated from github.com) commented 2025-10-30 19:50:44 +01:00
Copy Link

Come mai è stata aggiunta una funzione in più? Non bastava fare tutto nella funzione precedente?
Inoltre, come mai vengono passati tutti questi tipi di dati?

Come mai è stata aggiunta una funzione in più? Non bastava fare tutto nella funzione precedente? Inoltre, come mai vengono passati tutti questi tipi di dati?
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
volume_24h: float = 0.0
currency: str = ""
provider: str = ""
Berack96 (Migrated from github.com) commented 2025-10-30 19:50:58 +01:00
Copy Link

Come mai è stato aggiunto il provider direttamente nel product? Il provider è già passato come input nel dizionario e bastava dare in output una lista (o una stringa) con tutti i providers.

Come mai è stato aggiunto il provider direttamente nel product? Il provider è già passato come input nel dizionario e bastava dare in output una lista (o una stringa) con tutti i providers.
src/app/api/tools/market_tool.py
Berack96 (Migrated from github.com) commented 2025-10-30 19:03:16 +01:00
Copy Link

Questo non ha senso, come era prima già creava un dizionario dato con il nome del provider e, per ognuno, una lista di valori

Questo non ha senso, come era prima già creava un dizionario dato con il nome del provider e, per ognuno, una lista di valori
src/app/api/tools/symbols_tool.py
Berack96 (Migrated from github.com) commented 2025-10-30 20:00:27 +01:00
Copy Link

La tabella non viene creata in questo modo. Ha molti altri dati interni e non solo Symbol e Name.

Il try è effettivamente inutile, dato che una eccezione può essere sollevata solo da read_csv se il file non esiste.
Ma se il file non esiste allora crea un dataframe nuovo.
L'unico caso in cui può essere utile è se il csv è corrotto. Ma ciò non può succedere dato che è seguito da git.

Rimettere come prima la creazione di quest'ultima.

La tabella non viene creata in questo modo. Ha molti altri dati interni e non solo Symbol e Name. Il try è effettivamente inutile, dato che una eccezione può essere sollevata solo da read_csv se il file non esiste. Ma se il file non esiste allora crea un dataframe nuovo. L'unico caso in cui può essere utile è se il csv è corrotto. Ma ciò non può succedere dato che è seguito da git. Rimettere come prima la creazione di quest'ultima.
Berack96 (Migrated from github.com) commented 2025-10-30 20:07:40 +01:00
Copy Link

Il try qui non ha molto senso, dato che non potrebbe mai sollevare una eccezione.

L'unico caso in cui potrebbe succedere è se la query è una regex non valida, ma basta aggiungere regex=False alla funzione pandas

Il try qui non ha molto senso, dato che non potrebbe mai sollevare una eccezione. L'unico caso in cui potrebbe succedere è se la query è una regex non valida, ma basta aggiungere regex=False alla funzione pandas
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:28:15 +01:00
src/app/api/tools/market_tool.py
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:28:14 +01:00
Copy Link

Pensavo di averlo ripristinato, mi è sfuggito. Era una prova. Comunque prima usava la get_products mentre ora compone un dizionario iterando con la get_product, è la stessa cosa.

Pensavo di averlo ripristinato, mi è sfuggito. Era una prova. Comunque prima usava la `get_products` mentre ora compone un dizionario iterando con la `get_product`, è la stessa cosa.
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:28:54 +01:00
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:28:54 +01:00
Copy Link

Come prima, dimenticanza da esperimento.

Come prima, dimenticanza da esperimento.
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:32:38 +01:00
src/app/agents/prompts/team_leader.md
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:32:38 +01:00
Copy Link

Rispondo solo a uno. Come faccio a cambiare il prompt se ne ho bisogno? Devo davvero essere così rigido? Se cambio branch per cambiarlo poi non posso comunque averlo nel mio branch su cui sto lavorando.

Rispondo solo a uno. Come faccio a cambiare il prompt se ne ho bisogno? Devo davvero essere così rigido? Se cambio branch per cambiarlo poi non posso comunque averlo nel mio branch su cui sto lavorando.
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:32:54 +01:00
src/app/api/tools/symbols_tool.py
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:32:54 +01:00
Copy Link

Non mi ricordo, non ho memoria di quella cosa

Non mi ricordo, non ho memoria di quella cosa
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:32:58 +01:00
src/app/api/tools/symbols_tool.py
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:32:58 +01:00
Copy Link

Non mi ricordo, non ho memoria di quella cosa

Non mi ricordo, non ho memoria di quella cosa
Simo93-rgb (Migrated from github.com) reviewed 2025-10-30 20:34:42 +01:00
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
volume_24h: float = 0.0
currency: str = ""
provider: str = ""
Simo93-rgb (Migrated from github.com) commented 2025-10-30 20:34:42 +01:00
Copy Link

Mi serviva nell'aggregate. Ma a pensarci ora quel dato era come chiave del dizionario. Vorrei giustificartelo ma adesso mi sento perso. Sono stanco e non vorrei dirti altre stronzate. Scusa, mi odierai...

Mi serviva nell'aggregate. Ma a pensarci ora quel dato era come chiave del dizionario. Vorrei giustificartelo ma adesso mi sento perso. Sono stanco e non vorrei dirti altre stronzate. Scusa, mi odierai...
Berack96 (Migrated from github.com) reviewed 2025-10-30 23:22:14 +01:00
src/app/agents/prompts/team_leader.md
Berack96 (Migrated from github.com) commented 2025-10-30 23:22:14 +01:00
Copy Link

Esattamente, dato che quelle modifiche sono di un altro branch devono stare su un altro branch.
D'altronde qui stai facendo la parte di aggregazione non i prompt degli agenti.

Poi, in realtà non serve che tu rimuova questa parte da questo PR, però sappi che ci sono dei merge conflict su questo file e sugli altri, dato che questo branch è vecchio rispetto al main.

Se volevi avere le modifiche che ci sono ora sul main basta che, prima di modificare qualcosa, o dopo aver committato tutto, tu faccia (sempre su questo branch):

git merge main
Esattamente, dato che quelle modifiche sono di un altro branch devono stare su un altro branch. D'altronde qui stai facendo la parte di aggregazione non i prompt degli agenti. Poi, in realtà non serve che tu rimuova questa parte da questo PR, però sappi che ci sono dei merge conflict su questo file e sugli altri, dato che questo branch è vecchio rispetto al main. Se volevi avere le modifiche che ci sono ora sul main basta che, prima di modificare qualcosa, o dopo aver committato tutto, tu faccia (sempre su questo branch): ```bash git merge main ```
Berack96 (Migrated from github.com) reviewed 2025-10-30 23:24:14 +01:00
src/app/api/core/markets.py
@@ -13,43 +13,56 @@ class ProductInfo(BaseModel):
price: float = 0.0
volume_24h: float = 0.0
currency: str = ""
provider: str = ""
Berack96 (Migrated from github.com) commented 2025-10-30 23:24:14 +01:00
Copy Link

Guarda che non odio nessuno, sono solo più pistino di Copilot.
Io chiedo solo le modifiche che sono state effettuate solo perchè le avrei fatte in un altro modo o non mi aspetto qualche cosa che è stata fatta.

Guarda che non odio nessuno, sono solo più pistino di Copilot. Io chiedo solo le modifiche che sono state effettuate solo perchè le avrei fatte in un altro modo o non mi aspetto qualche cosa che è stata fatta.
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 62-aggregazione-market-product-non-corretta:62-aggregazione-market-product-non-corretta
git checkout 62-aggregazione-market-product-non-corretta
Sign in to join this conversation.
Reviewers
No Reviewers
copilot-pull-request-reviewer[bot]
Simo93-rgb
Berack96
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 important
This is a core feature that must be worked on as soon as possible
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Berack
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Berack/upo-app-agents#67
Delete Branch "62-aggregazione-market-product-non-corretta"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?