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.

Sign up for GitHub

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

Jump to bottom

Remove combined config #485

Merged
merged 63 commits into from
Jun 9, 2022
Merged

Remove combined config #485

merged 63 commits into from
Jun 9, 2022

Conversation

jannikgro
Copy link
Collaborator

closes #484

@jannikgro jannikgro added this to the Recommerce to main milestone May 26, 2022
@jannikgro jannikgro self-assigned this May 26, 2022
@jannikgro jannikgro changed the title solved #484 without check Remove combined config May 26, 2022
felix-20
felix-20 reviewed May 26, 2022
View reviewed changes
Copy link
Collaborator

@felix-20 felix-20 left a comment •
edited by NikkelM
Loading

Choose a reason for hiding this comment

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

When I do recommerce --get-defaults-unpack I get the following files:
grafik
Is this correct?
Not sure, what you have done here, but this doesnt seem to be right. Maybe I am doing something wrong?

Edit Nikkel: Fixed

recommerce/default_data/configuration_files/market_config.json Outdated Show resolved Hide resolved
recommerce/default_data/configuration_files/rl_config.json Outdated Show resolved Hide resolved
@NikkelM NikkelM marked this pull request as ready for review June 3, 2022 08:53
@NikkelM NikkelM requested a review from nick-bessin June 3, 2022 12:25
nick-bessin
nick-bessin requested changes Jun 4, 2022
View reviewed changes
Copy link
Collaborator

@nick-bessin nick-bessin left a comment

Choose a reason for hiding this comment

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

You've did a great job on this PR!

Regarding the README-file, we should also adapt the line where we've stated that by unpacking the default-data the current json-files won't be overridden anymore.
(Could be possible that there are some other out-dated phrases in the README-file that have to be adapted.)

P.S.: I'm sorry @felix-20, but I don't get all the new stuff that you've implemented for our webserver application. 😅

recommerce/configuration/environment_config.py Show resolved Hide resolved
recommerce/configuration/hyperparameter_config.py Outdated Show resolved Hide resolved
recommerce/configuration/hyperparameter_config.py Outdated Show resolved Hide resolved
recommerce/configuration/hyperparameter_config.py Outdated Show resolved Hide resolved
recommerce/configuration/hyperparameter_config.py Show resolved Hide resolved
tests/test_agent_monitoring/test_am_configuration.py Show resolved Hide resolved
tests/test_hyperparameter_config_rl.py Outdated Show resolved Hide resolved
webserver/alpha_business_app/buttons.py Outdated Show resolved Hide resolved
webserver/alpha_business_app/config_merger.py Show resolved Hide resolved
webserver/alpha_business_app/config_parser.py Show resolved Hide resolved
@NikkelM
Copy link
Collaborator

NikkelM commented Jun 4, 2022

You've did a great job on this PR!

Regarding the README-file, we should also adapt the line where we've stated that by unpacking the default-data the current json-files won't be overridden anymore. (Could be possible that there are some other out-dated phrases in the README-file that have to be adapted.)

P.S.: I'm sorry @felix-20, but I don't get all the new stuff that you've implemented for our webserver application. 😅

Unpacking the default data using -gdu should still override the current files. Could you check again?

NikkelM and others added 4 commits June 4, 2022 15:02
@NikkelM
Fixed config validation
99ab681
@NikkelM
Review feedback by @SinNeax
53d79f9
@felix-20
remove config_is_final
8839f90
@felix-20 @NikkelM
improved prefill (#496)
ab704ca 
* improved prefill with consideration of the current formdata

* remove debug statements

* remove print debug

* Added lxml dependency

* Removed debug comment

Co-authored-by: Nikkel Mollenhauer <[email protected]>
jannikgro
jannikgro commented Jun 7, 2022
View reviewed changes
Copy link
Collaborator Author

@jannikgro jannikgro left a comment

Choose a reason for hiding this comment

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

Whow, it was really much to do in the webserver!

recommerce/configuration/hyperparameter_config.py Show resolved Hide resolved
recommerce/configuration/hyperparameter_config.py Outdated Show resolved Hide resolved
recommerce/configuration/json_configurable.py Show resolved Hide resolved
recommerce/market/circular/circular_sim_market.py Show resolved Hide resolved
recommerce/market/linear/linear_sim_market.py Show resolved Hide resolved
tests/utils_tests.py Show resolved Hide resolved
@NikkelM NikkelM mentioned this pull request Jun 8, 2022
Open
@jannikgro jannikgro merged commit a910d62 into development Jun 9, 2022
@jannikgro jannikgro deleted the 484-configuration-remove-combined-config branch June 9, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felix-20 felix-20 felix-20 left review comments

@NikkelM NikkelM NikkelM left review comments

@nick-bessin nick-bessin nick-bessin approved these changes

Assignees

@felix-20 felix-20

@jannikgro jannikgro

@NikkelM NikkelM

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Configuration] Remove combined config
4 participants
@jannikgro @NikkelM @felix-20 @nick-bessin