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

Load testing #50

Closed
wants to merge 10 commits into from
Closed

Load testing #50

wants to merge 10 commits into from

Conversation

jmorel
Copy link
Contributor

@jmorel jmorel commented Dec 19, 2019

A few tests to try injecting larger and larger compute plans into the platform and making sure the platform can handle them.

Use pytest tests/test_load.py -rs -v --durations=0 to run these tests only.


factory, network = global_execution_env
session_1 = network.sessions[0].copy()
session_2 = network.sessions[0].copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
session_2 = network.sessions[0].copy()
session_2 = network.sessions[1].copy()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That was a big typo. I fixed it everywhere.

Copy link
Contributor

@samlesu samlesu left a comment

Choose a reason for hiding this comment

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

Thanks @jmorel ! Really good idea to implement these 3 use cases.

The 3 tests have exactly the same pattern.
Would it be possible to create a single test depending on a parametrized fixture called compute_plans (without sacrificing readability)?
https://docs.pytest.org/en/latest/fixture.html#parametrizing-fixtures

Not sure it will be better, but it looks fun to try ;)

algo=algo,
dataset=dataset,
data_samples=[dataset.train_data_sample_keys[0]],
in_models=[previous_tuple] if previous_tuple else None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor and won't change anything:

Suggested change
in_models=[previous_tuple] if previous_tuple else None,
in_models=[previous_tuple] if previous_tuple else [],



@pytest.mark.parametrize('compute_plan_size', COMPUTE_PLAN_SIZES)
def test_load_single_node_1_branch(compute_plan_size, global_execution_env):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really clear what's the goal of this test.

The goal of this test is to check that we can create a compute plan with a "lot of tuples".
That's the reason why it is cancelled shortly after creation.

Strictly speaking, I'm not sure it's a load test. This test should not take long to complete.

Comment on lines 39 to 48

cp = session.cancel_compute_plan(cp.compute_plan_id)
assert cp.status == assets.Status.canceled

cp = cp.future().wait()
assert cp.status == assets.Status.canceled

first_tuple = cp.list_traintuple()[0]
assert first_tuple.rank == 0
assert first_tuple.status == assets.Status.done
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the goal of this test is only to check that compute plan can be added and that the first tuple has been executed properly.

What about replacing the selected lines with:

    cp = session.cancel_compute_plan(cp.compute_plan_id).future().wait()
    assert cp.status == assets.Status.canceled

It will make the test simpler to read and it will be easier to understand what is tested.

Same for the 3 other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely!


factory, network = global_execution_env
session_1 = network.sessions[0].copy()
session_2 = network.sessions[0].copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
session_2 = network.sessions[0].copy()
session_2 = network.sessions[1].copy()

@jmorel
Copy link
Contributor Author

jmorel commented Jun 5, 2020

Since this is not necessary anymore and since we don't plan on running it regularly there's no point in keeping the PR open. I'm closing it.

@jmorel jmorel closed this Jun 5, 2020
@AlexandrePicosson AlexandrePicosson deleted the load_testing branch September 5, 2022 13:28
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.

2 participants