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 run on macOS #108

Merged
merged 9 commits into from
Feb 23, 2022
Merged

Enable run on macOS #108

merged 9 commits into from
Feb 23, 2022

Conversation

sharp-pixel
Copy link
Contributor

Description

Enables run on macOS.

Note 1: due to OpenSearch 1.0.1 not being available for Apple Silicon, the integration tests fail on a Mac M1. There is not much that I can do here.

Note 2: this PR also fixes the templates to run on Amazon OpenSearch Service with OpenSearch 1.1, as the _doc is now an error.

Issues Resolved

#89

Check List

  • New functionality includes testing
    • All unit and integration tests pass
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dblock
Copy link
Member

dblock commented Feb 7, 2022

Let's also add Darwin to CI?

@sharp-pixel
Copy link
Contributor Author

Anything I can do from my side?

@dblock
Copy link
Member

dblock commented Feb 11, 2022

@sharp-pixel
Copy link
Contributor Author

Added

dblock
dblock previously requested changes Feb 11, 2022
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

How about this?

.github/workflows/main.yml Outdated Show resolved Hide resolved
@sharp-pixel sharp-pixel requested a review from dblock February 11, 2022 19:42
@sharp-pixel
Copy link
Contributor Author

Hum, time based unit tests... interesting.
Python time.sleep's implementation on macOS seems not to be accurate enough: https://stackoverflow.com/questions/1133857/how-accurate-is-pythons-time-sleep
Also these tests would be highly dependent of the underlying system.
Now the problem is that is the system's time.sleep is not accurate enough and this is used for the actual benchmarking then the results would be unreliable, and it would be impossible to compare benchmarks run from different environments.

A suggestion would be to remove these tests on the build system, but run them at start of an actual benchmark to validate that the system running it is within accuracy limits. Open for discussion.

@dblock
Copy link
Member

dblock commented Feb 14, 2022

Generally, I recommend disabling unpredictable/unreliable tests and opening an issue to replace/fix them to be 100% reliable.

Copy link
Contributor

@engechas engechas left a comment

Choose a reason for hiding this comment

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

Based on the mac CI unit tests, there's other tests that are time specific and will need to be disabled as well

FAILED tests/client_test.py::RequestContextManagerTests::test_propagates_nested_context
FAILED tests/worker_coordinator/runner_test.py::CompositeTests::test_adds_request_timings

Rest LGTM

engechas
engechas previously approved these changes Feb 17, 2022
@engechas engechas dismissed their stale review February 17, 2022 18:01

Looking further into the ramifications of removing _doc

@sharp-pixel
Copy link
Contributor Author

The _doc will be a problem for Elasticsearch < v6.7, and between 6.7 and 7.x, we would need to set include_type_name=false explicitely. This is the default after Elasticsearch 7.0 and for OpenSearch.

@sharp-pixel
Copy link
Contributor Author

sharp-pixel commented Feb 17, 2022

If we want to still support older versions, we would need two versions of the templates, check version and use the correct ones.

@engechas
Copy link
Contributor

It looks like include_type_name based on version was accounted for here but doesn't work with OpenSearch metrics stores since the version is reported as 1.

I see two options:

  1. Remove _doc, forcing metrics stores to be ES7+ or OpenSearch
  2. Refactor the logic here to send include_type_name=true for OpenSearch metrics stores

Either option seems fine to me. Curious to hear others' thoughts

- Upgraded versions of Python that are aware of the Apple Silicon architecture
- Use mock to patch cpu_arch for `test_release_repo_config_with_user_url` so that `x64` is returned
- Fix message to launch workload
- Update templates for OpenSearch output

Signed-off-by: Cédric Pelvet <[email protected]>
Signed-off-by: Cédric Pelvet <[email protected]>
@sharp-pixel
Copy link
Contributor Author

It looks like include_type_name based on version was accounted for here but doesn't work with OpenSearch metrics stores since the version is reported as 1.

I see two options:

1. Remove `_doc`, forcing metrics stores to be ES7+ or OpenSearch

2. Refactor the logic [here](https://github.com/opensearch-project/opensearch-benchmark/blob/ba96b9a262a303a739e39296ec676e6908c21905/osbenchmark/metrics.py#L68-L72) to send `include_type_name=true` for OpenSearch metrics stores

Either option seems fine to me. Curious to hear others' thoughts

I made some changes:

  • I restored the _doc in the templates for now.
  • I added a new _cluster_distribution field in OsClient that holds the distribution (either 'opensearch' or 'elasticsearch'), that way we can differentiate versions for OpenSearch from versions for Elasticsearch.

osbenchmark/metrics.py Outdated Show resolved Hide resolved
osbenchmark/metrics.py Outdated Show resolved Hide resolved
Copy link
Contributor

@engechas engechas left a comment

Choose a reason for hiding this comment

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

LGTM

Integration tests using an OpenSearch metrics store with these changes and the changes in #119 are passing

@engechas
Copy link
Contributor

I have created followup issues to break the DCO out into it's own file and to re-evalutate the disabled unit tests: #120 #121

Merging this PR, thanks!

@engechas engechas dismissed dblock’s stale review February 23, 2022 21:18

Changes were addressed, follow up issue created to break the DCO check into a separate file #120

@engechas engechas merged commit f640f98 into opensearch-project:main Feb 23, 2022
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.

4 participants