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

[Metric] Add ray metrics implementation #2749

Merged
merged 8 commits into from
Mar 1, 2022

Conversation

zhongchun
Copy link
Contributor

@zhongchun zhongchun commented Feb 25, 2022

What do these changes do?

This pr adds ray metrics implementation which can be used when mars on ray. And we can use ray's metric backend to record and report metrics data to external system in this scenario.

We can use ray metrics as follows:

from mars.metric.api import init_metrics, Metrics

init_metrics({"metric": {"backend": "ray"}})
c = Metrics.counter("test_counter", "A test counter", ("service", "tenant"))
c.record(1)

g = Metrics.gauge("test_gauge", "A test gauge")
g.record(1)

m = Metrics.meter("test_meter")
m.record(1)

h = Metrics.histogram("test_histogram")
h.record(1)

Related issue number

Issue #2743

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

@wjsi
Copy link
Member

wjsi commented Feb 25, 2022

Need to add tests under Ray. Try use require_ray decorator to wrap ray-specific tests. Also rename RayMetric to RayMetricMixin to make things consistent.

@zhongchun
Copy link
Contributor Author

Need to add tests under Ray. Try use require_ray decorator to wrap ray-specific tests. Also rename RayMetric to RayMetricMixin to make things consistent.

Thanks for your reminder. There are ray metrics tests in mars/metric/backends/ray/tests/test_ray_metric.py. I'll fix the name.

@qinxuye qinxuye mentioned this pull request Feb 25, 2022
4 tasks
@qinxuye qinxuye added this to the v0.9.0b2 milestone Feb 25, 2022
@zhongchun zhongchun force-pushed the feat-add-ray-metric branch 2 times, most recently from 5a719c6 to a459abc Compare February 25, 2022 09:18
@zhongchun zhongchun changed the title Add ray metrics implementation [Metric]Add ray metrics implementation Feb 25, 2022
@zhongchun zhongchun force-pushed the feat-add-ray-metric branch 2 times, most recently from 2215089 to 2cf7ca8 Compare February 28, 2022 06:25
@zhongchun zhongchun mentioned this pull request Mar 1, 2022
2 tasks

class RayMetricMixin(AbstractMetric):
def _init(self):
if ray_metrics:
Copy link
Member

@wjsi wjsi Mar 1, 2022

Choose a reason for hiding this comment

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

Append a # pragma: no branch at the end of the line to avoid branch coverage report. What's more, it is recommended to use ray_metrics is None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Thanks!

def _record(self, value=1, tags: Optional[Dict[str, str]] = None):
if RAY_GAUGE_SET_AVAILABLE:
self._metric.set(value, tags)
elif ray_metrics:
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it.

Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wjsi wjsi left a comment

Choose a reason for hiding this comment

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

LGTM

@wjsi wjsi merged commit f6986f2 into mars-project:master Mar 1, 2022
qinxuye pushed a commit to hekaisheng/mars that referenced this pull request Mar 1, 2022
@zhongchun zhongchun changed the title [Metric]Add ray metrics implementation [Metric] Add ray metrics implementation Mar 4, 2022
chaokunyang pushed a commit to chaokunyang/mars that referenced this pull request May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants