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

Sagemaker-Metrics: Add sagemaker metrics #7617

Merged
merged 13 commits into from Apr 29, 2024

Conversation

YHallouard
Copy link
Contributor

No description provided.

@YHallouard
Copy link
Contributor Author

Hi, I put a lot of cast inside my PR and I'm don't very like it...

Do you prefer using cast, "# type: ignore", a Typed Dict (but will force typing_extension for old python version), a pydantic model or create a Metric Object ?

@YHallouard
Copy link
Contributor Author

YHallouard commented Apr 22, 2024

Hi @bblommers, I hope you're doing well :)

I'm a little bit lost, my test suite is failing on test server. I can replicate it on local, it seems that the sagemaker metrics backend is not created in server mode. Do you have any idea ?

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 97.18310% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.30%. Comparing base (9aef694) to head (e363dd7).
Report is 220 commits behind head on master.

Files Patch % Lines
moto/moto_server/werkzeug_app.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7617      +/-   ##
==========================================
- Coverage   95.88%   94.30%   -1.58%     
==========================================
  Files         843     1086     +243     
  Lines       82578    92452    +9874     
==========================================
+ Hits        79178    87187    +8009     
- Misses       3400     5265    +1865     
Flag Coverage Δ
servertests 29.44% <57.74%> (-3.08%) ⬇️
unittests 94.27% <97.18%> (-1.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

The routing needed some additional logic to deal with the multiple Sagemaker services - that's fixed now.

LGTM - thanks @YHallouard!

@bblommers bblommers added this to the 5.0.7 milestone Apr 29, 2024
@bblommers bblommers merged commit a9866b0 into getmoto:master Apr 29, 2024
39 of 40 checks passed
@YHallouard
Copy link
Contributor Author

The routing needed some additional logic to deal with the multiple Sagemaker services - that's fixed now.

LGTM - thanks @YHallouard!

Wow, tricky ! Thank you very much for the help ! You rock ! @bblommers

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.

None yet

2 participants