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

Feat/amm migration script & in-flight config disable #252

Merged

Conversation

petioptrv
Copy link

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 adds a config migration step for AMM, as well as disables the ability to configure a Pydantic strategy config while the strategy is running (which currently is not fully fleshed out and may lead to errors).

Tests performed by the developer:

  • Manually tested the changes.

Tips for QA testing:

  • For AMM migration
    • The migration should be able to migrate any configuration combination produced by the client in development
      • It is not required to handle cases where the user changed the values themselves to something that doesn't make sense (e.g. if a value is required, but the user removed it)
    • Please test all variations of the "modes"
  • For the in-flight config disable
    • Please test that legacy configs act as they previously did
    • Test that the new Pydantic configs can be configured while the strategy is stopped, but must not be configurable when the strategy is running.
    • The global configs should be editable at all times.

@petioptrv petioptrv requested review from mhrvth and a team June 2, 2022 14:17
@petioptrv petioptrv self-assigned this Jun 2, 2022
@RC-13
Copy link

RC-13 commented Jun 3, 2022

Tested and confirmed working as expected when running from source, pending test on docker build. Currently blocked by the following error when starting a docker instance after creating a docker image based from the feature branch:

Traceback (most recent call last):
  File "bin/hummingbot_quickstart.py", line 18, in <module>
    from hummingbot.client.config.config_helpers import (
ImportError: cannot import name 'create_yml_files' from 'hummingbot.client.config.config_helpers' (/home/hummingbot/hummingbot/client/config/config_helpers.py)

Copy link

@mhrvth mhrvth left a comment

Choose a reason for hiding this comment

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

LGTM

@gerald-hb
Copy link

Still hitting the issue Clinton flagged on creating a docker image.
image
This block testing on dockerbuild

  File "bin/hummingbot_quickstart.py", line 18, in <module>
    from hummingbot.client.config.config_helpers import (
ImportError: cannot import name 'create_yml_files' from 'hummingbot.client.config.config_helpers' (/home/hummingbot/hummingbot/client/config/config_helpers.py)```

@RC-13
Copy link

RC-13 commented Jun 5, 2022

Re-tested the PR and was able to create a docker container without the error mentioned above, however, got the following message in the background:
image
Despite this message in the background, I was able to start the strategies (including strategy that uses Kraken connector).

@petioptrv
Copy link
Author

@RC-13, I have addressed the issue you flagged. Those messages should not appear anymore. Please test the migration and use of celo and kraken one more time. They should now migrate properly.

Copy link

@gerald-hb gerald-hb left a comment

Choose a reason for hiding this comment

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

Latest commit looks good to me. I no longer see the last flagged error by Clinton. No notable error as well on connection/balance and running strategies on source and docker build.

Test Summary
-Build a docker image based on the feature branch
-Create a docker instance using the image
-Start the client/setup password
-Create/Import strategy
-Start the strategy
-Configure parameter on the fly
-Stop the strategy and configure the global parameters
-Restart the strategy
-Create a new Avellaneda MM strategy and start
-Configure the parameter while the strategy is running
-Stop the strategy and configure the parameters
-Restart the strategy

@petioptrv petioptrv merged commit b409311 into feat/migrate_global_config_to_pydantic Jun 9, 2022
@petioptrv petioptrv deleted the feat/amm_migration_script branch April 5, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants