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

enable AWS dispatcher tests #494

Merged
merged 23 commits into from
Dec 16, 2020

Conversation

maikia
Copy link
Contributor

@maikia maikia commented Dec 14, 2020

As for now the AWS worker is poorly tested.

This PR:

  1. adds the necessary tools to test the AWS from the dispatcher level (ie creates the aws event in the test database session)
  2. it adds a sample test for not launched instance due to authorization issue
  3. it closes BUG dispatcher failing on lack of instance #492 by checking if the instance is assigned to the worker before killing the worker
  4. it also cleans up the tests of aws worker to use the same type of template (and places it in the same locations) as it is done for the conda worker

sorry for such a long PR. It was hard to cut it once it started to grow :-/

@maikia
Copy link
Contributor Author

maikia commented Dec 14, 2020

@rth I would be curious about your opinion on the config file.
Here I am placing the submissions_dir, predictions_dir and logs_dir in both worker and ramp for local and remote separately. This might need to be clarified in the docs if you agree with that approach.

@maikia maikia requested a review from lucyleeow December 14, 2020 14:36
@maikia maikia requested a review from rth December 14, 2020 14:55
Copy link
Collaborator

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thanks a ton @maikia !

did you spot any issue doing this?

sandbox_dir: starting_kit
submissions_dir: /tmp/databoard_test/submissions
predictions_dir: /tmp/databoard_test/preds
logs_dir: /tmp/databoard_test/log
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe a bit dangerous that the system as /tmp folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm. I used the exactly the same directories for submissions, preds and log as it is already set for ramp_config_iris.yml (ie iris running on conda). boston_housing is also using /tmp folder. If you have alternative suggestion we should change it for all three

Copy link
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

Very useful to have the tests!

ramp-engine/ramp_engine/tests/test_dispatcher.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #494 (8d5b9c6) into master (2e12b8d) will increase coverage by 0.01%.
The diff coverage is 98.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   93.61%   93.62%   +0.01%     
==========================================
  Files          99       99              
  Lines        8552     8587      +35     
==========================================
+ Hits         8006     8040      +34     
- Misses        546      547       +1     
Impacted Files Coverage Δ
ramp-database/ramp_database/tests/test_cli.py 100.00% <ø> (ø)
ramp-engine/ramp_engine/dispatcher.py 98.18% <ø> (ø)
ramp-utils/ramp_utils/testing.py 87.50% <50.00%> (-12.50%) ⬇️
...abase/ramp_database/model/tests/test_submission.py 100.00% <100.00%> (ø)
ramp-database/ramp_database/testing.py 100.00% <100.00%> (ø)
...abase/ramp_database/tools/tests/test_submission.py 100.00% <100.00%> (ø)
ramp-engine/ramp_engine/tests/test_aws.py 86.44% <100.00%> (ø)
ramp-engine/ramp_engine/tests/test_daemon.py 100.00% <100.00%> (ø)
ramp-engine/ramp_engine/tests/test_dispatcher.py 100.00% <100.00%> (ø)

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 2e12b8d...8d5b9c6. Read the comment docs.

@maikia
Copy link
Contributor Author

maikia commented Dec 15, 2020

@tomMoral would you mind having a look? thx

Copy link
Collaborator

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

A couple of cosmit and one question to extend the tests but overall it looks pretty good!
Thx @maikia

ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
ramp-database/ramp_database/model/tests/test_submission.py Outdated Show resolved Hide resolved
@@ -65,6 +65,7 @@
from ramp_database.tools.submission import submit_starting_kits

HERE = os.path.dirname(__file__)
ID_SUBMISSION = 7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it supposed to be the same as the one in ramp-database/ramp_database/model/tests/test_submission.py?
If yes, maybe define it uniquely in ramp-database/ramp_database/testing.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be... but I think it's easier to debug if you have the variable in the same testing file..
if it's very important to you I will change but I am slightly leaning against it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not that important. It is just that when you have constant, I find it better to not have multiple definitions. But if it is not critical (as it is in the tests), do what you think is the best :)

@maikia maikia mentioned this pull request Dec 15, 2020
@maikia
Copy link
Contributor Author

maikia commented Dec 16, 2020

@tomMoral could you please merge if you are ok with the changes?
Thx

@agramfort agramfort merged commit f7211c4 into paris-saclay-cds:master Dec 16, 2020
@agramfort
Copy link
Collaborator

thanks @maikia and @tomMoral !

maikia added a commit that referenced this pull request Dec 16, 2020
* updated the way of setting the aws config template

* init session toy db for aws

* adding an event iris to run on aws for test db

* sample test for not launching worker

* catching correctly the wrong credentials error and checking if worker has assigned an instance before killing the worker

* clean up

* added aws config file for iris (tests)

* cleanup

* update the daemon test to account for new aws event

* update the submission tests to work with the new event

* clean up

* update the tests

* update the tests

* spelling

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* Update ramp-database/ramp_database/model/tests/test_submission.py

Co-authored-by: Thomas Moreau <[email protected]>

* test if all the submissions are still in the new state

* cleanup

Co-authored-by: Thomas Moreau <[email protected]>
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.

BUG dispatcher failing on lack of instance
4 participants