-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor / adapts Avellaneda config map for pydantic #96
refactor / adapts Avellaneda config map for pydantic #96
Conversation
The config map introduced in this PR is a proof of concept for the new approach to configs using pydantic. It adopts @aarmoa's suggestion to use nested models as a method of solving the interdependencies issues present with some config variables.
This pull request has been linked to Shortcut Story #21823: Create Avellaneda configuration schema. |
class ExecutionTimeframe(str, ClientConfigEnum): | ||
infinite = "infinite" | ||
from_date_to_date = "from_date_to_date" | ||
daily_between_times = "daily_between_times" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enum required? Wouldn't it be better to have the a class variable "description" or "title" in each of the model classes to store the string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced the enum for a dictionary as suggested below
hummingbot/strategy/avellaneda_market_making/avellaneda_market_making_config_map_pydantic.py
Outdated
Show resolved
Hide resolved
hummingbot/strategy/avellaneda_market_making/avellaneda_market_making_config_map_pydantic.py
Outdated
Show resolved
Hide resolved
hummingbot/strategy/avellaneda_market_making/avellaneda_market_making_config_map_pydantic.py
Outdated
Show resolved
Hide resolved
hummingbot/strategy/avellaneda_market_making/avellaneda_market_making_config_map_pydantic.py
Outdated
Show resolved
Hide resolved
hummingbot/strategy/avellaneda_market_making/avellaneda_market_making_config_map_pydantic.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
…ydantic' into refactor/avellaneda_config_map_pydantic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a question to one of the previous comments and one more requested change to add a new line at the end of one file.
test/hummingbot/strategy/avellaneda_market_making/test_config.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
…ydantic' into refactor/avellaneda_config_map_pydantic
I've added a small cleanup commit that I noticed while taking another look at the class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes look good to me now. I think we should create a big feature PR with all changes related to the configuration update before sending the new functionality to the Foundation.
The config map introduced in this PR is a proof of concept for the new approach to configs using pydantic. It adopts @aarmoa's suggestion to use nested models as a method of solving the interdependencies issues present with some config variables.
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
…ydantic' into refactor/avellaneda_config_map_pydantic
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
This PR introduces a proof of concept for the new approach to defining config maps using pydantic.
It adopts @aarmoa's suggestion to use nested models to address the config variables' interdependencies problem.
Tests performed by the developer:
Tests cover the new functionalities introduced in the proof of concept, plus the functionality already previously covered by existing test.
This PR assumes that the model instance will be directly passed to the strategy, so it does not include a test to flatten the model dictionary in preparation for the current approach to the
init_params
function (config vars passed in as kwargs).Tips for QA testing:
N/A
[ch21823]