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

Refactor commands to take in configs instead of entities. Also Mocks. #567

merged 8 commits into from Nov 27, 2018


Copy link

@lossyrob lossyrob commented Nov 16, 2018


This PR is a refactor that is part of #551. It makes space for commands to implement logic that determines how configurations will turn into entities themselves and takes the entity creation logic out of the create_command method of the CommandConfig objects.

It also adds a tests/mock package, which is useful for testing entities and commands. Prior to this we didn't have a good way to run commands and experiments in unit tests in a way that we could check what happened - this new package allows for that.

Also - this removes the __deepcopy__ override on config builders that was happening as a performance booster. I realized during this that the deep copy override wasn't even happening on configs, where it could really make a difference, but on config builders - so I don't see a huge loss here. I removed because this is the second time I've seen it cause problems with un-pickle-able things.

Copy link

@codecov codecov bot commented Nov 16, 2018

Codecov Report

Merging #567 into develop will increase coverage by 1.14%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #567      +/-   ##
+ Coverage    69.16%   70.3%   +1.14%     
  Files          167     158       -9     
  Lines         7536    7179     -357     
- Hits          5212    5047     -165     
+ Misses        2324    2132     -192
Impacted Files Coverage Δ
rastervision/core/ 100% <ø> (ø) ⬆️
rastervision/backend/ 15.77% <ø> (ø) ⬆️
rastervision/command/ 100% <100%> (+54.54%) ⬆️
rastervision/command/ 100% <100%> (+58.33%) ⬆️
rastervision/command/ 77.88% <100%> (+15.49%) ⬆️
rastervision/command/ 100% <100%> (+61.53%) ⬆️
rastervision/command/ 80.24% <100%> (+16.39%) ⬆️
rastervision/command/ 81.81% <100%> (+18.58%) ⬆️
rastervision/command/ 100% <100%> (ø) ⬆️
rastervision/command/ 95.69% <100%> (ø) ⬆️
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28b2850...15777e9. Read the comment docs.

@lossyrob lossyrob requested a review from jamesmcclain Nov 16, 2018
Copy link

@jamesmcclain jamesmcclain left a comment

Looks self-consistent and tests pass.

@lossyrob lossyrob merged commit 014da06 into develop Nov 27, 2018
2 checks passed
@lossyrob lossyrob removed the review label Nov 27, 2018
@lossyrob lossyrob deleted the rde/refactor/command_config_create branch Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants