Skip to content
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

Epic/config management refactoring #5428

Merged

Conversation

petioptrv
Copy link
Contributor

Before submitting this PR, please make sure:

  • Your code builds clean without any errors or warnings
  • You are using approved title ("feat/", "fix/", "docs/", "refactor/")

A description of the changes proposed in the pull request:

This PR lays the foundation for a refactor of the configuration management of hummingbot.

The new approach uses pydantic models to define the configuration maps. Aside from built-in validation functionality, this approach also allows the automatic generation of JSON schemas which is a big first step in the direction of decoupling the bot from its interface. Another major step in that direction is significantly restricting the use of global variables when dealing with the global config map (now renamed to client config map) and the AllConnectorSettings class.

The approach to storing and retrieving secure configs has also been refactored. We no longer store secure configs in the client config map (former global config map). Those are only stored in the Security class (which is still unfortunately accessed globally). In addition, the secure values are no longer stored separate from non-secure configs — they are both part of the same config map and stored in the same yaml file.

Two of the strategies, AMM and XEMM, were already adapted to the new configs style. The rest remain to be adapted. To accommodate for legacy configs, some of the legacy config management code still remains. If the legacy functionality is an entire function, then the function has been renamed with a _legacy postfix. If the legacy functionality are lines of code part of an otherwise non-refactored routine, then a comment # legacy was added. This was done to later facilitate complete removal of legacy config code.

The Pydantic model now allows for nested config maps. This is used for two purposes. First, it is used to provide more organizational structure as with the color map, which is now a nested sub-model of the client config map. Second, it is used to provide conditional requirements for the configurations (i.e. "field foo is required only if field bar is est to the value X). An example of this is the AvellanedaMarketMakingConfigMap.execution_timeframe_mode field.

A legacy migration routine was included on the bot's startup, which will detect if the bot is being ran for the first time since the upgrade, and will guide the user through the migration of the refactored configs. The routine will start by creating a backup of the current state of the configs folder, and proceed from there. It will also alert the user if any config map migration failed along the way.

The folder structure of the configs has been further split into connectors and strategies for better organization.

Tests performed by the developer:

  • Unit tests for the covered changes.
  • Manual tests for a good portion of the changes.

Tips for QA testing:

  • Please re-run essentially a full test of the client. Some of the specific things I can think of are listed below.
  • All commands should be re-tested with the config command thoroughly re-tested, including secure configs.
  • Please make sure to test odd cases like celo as well.
  • Gateway should be tested.
  • XEMM and AMM configs can be tested superficially (no need to thoroughly test these as they have been tested previously and have not changed since then).

petioptrv and others added 30 commits February 18, 2022 10:16
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>
…ydantic' into refactor/avellaneda_config_map_pydantic
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
…ydantic' into refactor/avellaneda_config_map_pydantic
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
…pydantic

refactor / adapts Avellaneda config map for pydantic
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
Co-authored-by: Abel Armoa <30988000+aarmoa@users.noreply.github.com>
@petioptrv
Copy link
Contributor Author

PR Update: After creating Avellaneda strategy on Dev1.6.0 and importing it to this branch in the conf/strategies folder. We noticed after starting the bot and importing said strategy, the primary check indicated that "Validation of the config maps failed. The following errors were flagged.execution_timeframe_mode - field required". Is this the expected behavior?

@arnelhbot, generally yes, the execution_timeframe_mode is required and if not found, this is in fact what should happen. That being said, if, as you said, you had an existing configuration that was migrated and this occurred, this is less desirable. Can you share the original configuration file you used to produce this outcome please?

- Improves the printout for one of the migration messages.
- New configs now add the default as a suggestion on the prompt.
@petioptrv
Copy link
Contributor Author

@nikspz, I have addressed both concerns in the latest commit.

@nikspz
Copy link
Contributor

nikspz commented Jun 28, 2022

@petioptrv the file I used to reproduce that avellaneda issue.
conf_avellaneda_market_making_1.zip

@petioptrv
Copy link
Contributor Author

@petioptrv the file I used to reproduce that avellaneda issue. conf_avellaneda_market_making_1.zip

Thank you for that. I attempted to migrate the file with the current state of the branch and the process was successful; it was recognized as infinite. Are you still experiencing the faulty behaviour on your end?

@arnelhbot
Copy link
Contributor

PR Update: After creating Avellaneda strategy on Dev1.6.0 and importing it to this branch in the conf/strategies folder. We noticed after starting the bot and importing said strategy, the primary check indicated that "Validation of the config maps failed. The following errors were flagged.execution_timeframe_mode - field required". Is this the expected behavior?

@arnelhbot, generally yes, the execution_timeframe_mode is required and if not found, this is in fact what should happen. That being said, if, as you said, you had an existing configuration that was migrated and this occurred, this is less desirable. Can you share the original configuration file you used to produce this outcome please?

Here is the requested file to produce the outcome.
conf_avellaneda_mm.zip

@petioptrv
Copy link
Contributor Author

petioptrv commented Jun 28, 2022

PR Update: After creating Avellaneda strategy on Dev1.6.0 and importing it to this branch in the conf/strategies folder. We noticed after starting the bot and importing said strategy, the primary check indicated that "Validation of the config maps failed. The following errors were flagged.execution_timeframe_mode - field required". Is this the expected behavior?

@arnelhbot, generally yes, the execution_timeframe_mode is required and if not found, this is in fact what should happen. That being said, if, as you said, you had an existing configuration that was migrated and this occurred, this is less desirable. Can you share the original configuration file you used to produce this outcome please?

Here is the requested file to produce the outcome. conf_avellaneda_mm.zip

This one is also currently imported migrated successfully on my end

@nikspz
Copy link
Contributor

nikspz commented Jun 28, 2022

@petioptrv the file I used to reproduce that avellaneda issue. conf_avellaneda_market_making_1.zip

Thank you for that. I attempted to migrate the file with the current state of the branch and the process was successful; it was recognized as infinite. Are you still experiencing the faulty behaviour on your end?

commit 863b7f4
failed to start config after migrate and import

Steps:

  1. clone and install feature branch
  2. copy old config files to hummingbot_instance_name/conf/strategies
  3. start the instance
  4. verify mirgration successfully completed message
  5. import avellaneda strategy
  6. start the strategy

Actual:
5. Validation of the config maps failed. The following errors were flagged (execution_timeframe_mode)
6. Failed to start the strategy (need to modify execution_timeframe_mode manually first)

Expected:
Strategy imported and started without any issues after migration

28 06 migration avellaneda config failed to migrate execution_timeframe_mode


initial message about migration not going goes off-screen on small screen anymore 👌 , but that's not centered

Regarding default values fix, currently bot showing default values Before you tap enter on blank command
Also for some configs it returns weird import like that:
28 06 default values

28.06 default values showed Before you sent empty value
28 06 default values showed BEFORE you sent empty value

@petioptrv
Copy link
Contributor Author

@petioptrv the file I used to reproduce that avellaneda issue. conf_avellaneda_market_making_1.zip

Thank you for that. I attempted to migrate the file with the current state of the branch and the process was successful; it was recognized as infinite. Are you still experiencing the faulty behaviour on your end?

failed to start config after migrate and import

@nikspz, to migrate the original config properly, it should be directly under conf/, not under conf/strategies. The idea is that, when a user updates their hb instance, they will have old configs in conf/ that we want to migrate to conf/strategies in the new format.

@nikspz
Copy link
Contributor

nikspz commented Jun 28, 2022

@petioptrv Got it, and then I need to move them back to conf/strategies
let me retest

@petioptrv
Copy link
Contributor Author

petioptrv commented Jun 28, 2022

@petioptrv Got it, and then I need to move them back to conf/strategies
let me retest

Nope, the migration process will take care of that for you.

If the initial folder structure is:

- conf
    - old_strategy_conf.yml
    - old_connector_key.yml
    - old_connector_secret.yml

after the automatic migration, that should be:

- conf
    - strategies
        - new_strategy_conf.yml
    - connectors
        - new_connector.yml

@petioptrv
Copy link
Contributor Author

petioptrv commented Jun 28, 2022

initial message about migration not going goes off-screen on small screen anymore 👌 , but that's not centered

For this one, I see the dialog with the same spacing as the one before it:
Screen Shot 2022-06-28 at 4 50 45 PM
Screen Shot 2022-06-28 at 4 50 55 PM

@petioptrv
Copy link
Contributor Author

Regarding default values fix, currently bot showing default values Before you tap enter on blank command
Also for some configs it returns weird import like that:

I have addressed those in the last commit.

@nikspz
Copy link
Contributor

nikspz commented Jun 28, 2022

For this one, I see the dialog with the same spacing as the one before it: Screen Shot 2022-06-28 at 4 50 55 PM

What I mean for this screenshot: all text on the left part.
Anyway, it helped with text went off-screen issue on small-screen device. Thanks 👍

@arnelhbot
Copy link
Contributor

PR Update: After creating Avellaneda strategy on Dev1.6.0 and importing it to this branch in the conf/strategies folder. We noticed after starting the bot and importing said strategy, the primary check indicated that "Validation of the config maps failed. The following errors were flagged.execution_timeframe_mode - field required". Is this the expected behavior?

@arnelhbot, generally yes, the execution_timeframe_mode is required and if not found, this is in fact what should happen. That being said, if, as you said, you had an existing configuration that was migrated and this occurred, this is less desirable. Can you share the original configuration file you used to produce this outcome please?

Here is the requested file to produce the outcome. conf_avellaneda_mm.zip

This one is also currently imported migrated successfully on my end

Thanks @petioptrv for the clarification. I will retest this as soon as I start my shift with a clean environment.

@petioptrv
Copy link
Contributor Author

What I mean for this screenshot: all text on the left part.

I think I see what you mean here, but the printout generally does not adapt to the screen size, so I had to make it as wide as the currently existing prompts (like on the first screenshot), which I guess ended up clustering the text to the left side

@nikspz
Copy link
Contributor

nikspz commented Jun 28, 2022

@petioptrv
Tried migration of strategy configs and enrypted exchange files.
Migration of main LM connectors (binance, gateio, kucoin, ascendex) = OK 👍
migration huobi api keys showing error message after first start

Steps:

  1. clone and install feature branch
  2. copy old config and encrypted connectors (with Huobi files) files to hummingbot_instance_name/conf/
  3. start the instance
    Actual:
    Bot showed migration was successful message, however showing error message on the first start
    Anyway, balance check for Huobi works OK after migration. And that error message not showing on second start of the bot.
    image
    image

Tried to check pureMM creation:
Currently you Could add perpetual exchanges for spot connector

Steps:

  1. clone and install feature branch
  2. start the instance
  3. try to create simple pure market making strategy
  4. pay attention to the spot connector list

Actual:
Bot could add dydx perpetual, binance perpetual, coinflex perpetual, bybit perpetual as a spot connector.
user could create a strategy successfully using pureMM with perpetual connectors.

Expected:
Bot showing only spot connectors, user could add only spot connectotors there.

image
image

@petioptrv
Copy link
Contributor Author

@nikspz, I've addressed both issues in my latest commit.

@nikspz
Copy link
Contributor

nikspz commented Jun 29, 2022

@petioptrv Could you please resolve branch conflicts? (I think #5381 merged to development is the reason)

@petioptrv
Copy link
Contributor Author

@nikspz, conflicts resolved in the last commit

Copy link
Contributor

@arnelhbot arnelhbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test performed:

  • Cloned feature branch successfully
  • Started Client and created password
  • Connected certified exchanges APIs and successful checked balance
  • Verified triggered Killswitch
  • Imported multiple(Avellaneda, Pure_MM, Perpetual_MM, XEMM, AMM_Arbitrage,etc.) strategies successfully
  • Successfully created Avellaneda_MM and XEMM strategy with no issues
  • Trades executions matched between History, CSV, and exchange trade history
  • Able to generate-cert and gateway create with no issues
  • Connected Infura and verified
  • Connected Uniswap, Sushiswap, Pangolin and checked balance successfully
  • Verified Balance_paper correctly displayed Issue 5361
  • Created XEMM and AMM_Arbitrage strategy
  • Tested DEX/CEX and CEX/CEX successful
  • Started Client and monitored logs with no issues
  • No arbitrage opportunity was verified
  • History/-verbose displayed correctly
  • Confirmed CSV file and txhash to match on snowtrace
  • All are tested on Source AWS and Docker Build

@JeremyKono JeremyKono merged commit ea84d2b into hummingbot:development Jun 30, 2022
@petioptrv petioptrv deleted the epic/config_management_refactoring branch June 30, 2022 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants