-
Notifications
You must be signed in to change notification settings - Fork 831
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
chore: Scala client for certified events #2045
chore: Scala client for certified events #2045
Conversation
… from hadoop configuration. Changed it to retrieve rather from spark configuration
…on of the codes separately in Edog.
Hey @saileshbaidya 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary by GPT-4
The code changes include updates to the logging functionality in the SynapseML library. The updates include:
- Adding new classes and methods for handling JWT (JSON Web Tokens) and making HTTP requests.
- Updating existing logging methods to include a new parameter
logCertifiedEvent
which defaults tofalse
. This parameter is used to determine whether or not to log a certified event. - Adding new tests for the JWT token parser and host endpoint utilities.
The changes also remove an unnecessary newline character from two files: .gitignore
and environment.yml
.
Suggestions
The changes in this PR look good. However, there are a few suggestions for improvement:
-
In the
SynapseMLLogging.scala
file, thelogBase
method has been updated to include a new parameterlogCertifiedEvent
. It would be helpful to add comments explaining what this new parameter is for and how it affects the method's behavior. -
In the same file, the
logVerb
method has been updated to include two new parameters:columns
andlogCertifiedEvent
. Again, adding comments explaining these parameters would be beneficial. -
The newly added files under the
logging/Usage
directory lack comments explaining their purpose and functionality. Adding such comments would improve code readability and maintainability. -
In the test files (
FabricTokenParserTests.scala
,HostEndpointUtilsTests.scala
), it would be helpful to add more detailed descriptions of what each test is checking for in a comment above each test. -
There are several instances where lines of code have been removed from
.gitignore
,environment.yml
, andmanifest.yaml
. If these changes are intentional, it might be helpful to explain why in the PR description. -
Lastly, ensure that all new methods have appropriate unit tests to maintain code coverage.
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/Usage/UsageUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/Usage/UsageUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/common/WebUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/Usage/TokenUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/io/http/RESTHelpers.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/io/http/RESTHelpers.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/Usage/TokenUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/Usage/UsageConstants.scala
Outdated
Show resolved
Hide resolved
…eeded for token parsing.
… and few PR comments.
…eaning unused imports. 3) Closing unused resources like file handler, etc. 4) And fixing few scala style checks like calling convention for 0 parameter func, etc.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #2045 +/- ##
===========================================
- Coverage 87.02% 76.16% -10.86%
===========================================
Files 306 309 +3
Lines 16162 16259 +97
Branches 824 831 +7
===========================================
- Hits 14065 12384 -1681
- Misses 2097 3875 +1778
|
…sfully emitting telemetry.
/azp run |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/common/PlatformDetails.scala
Outdated
Show resolved
Hide resolved
…mon/PlatformDetails.scala
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/common/PlatformDetails.scala
Outdated
Show resolved
Hide resolved
…mon/PlatformDetails.scala
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/common/PlatformDetails.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/common/PlatformDetails.scala
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/fabric/CertifiedEventClient.scala
Outdated
Show resolved
Hide resolved
…ric/CertifiedEventClient.scala
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/fabric/CertifiedEventClient.scala
Outdated
Show resolved
Hide resolved
…ric/CertifiedEventClient.scala
core/src/main/scala/com/microsoft/azure/synapse/ml/logging/fabric/CertifiedEventClient.scala
Outdated
Show resolved
Hide resolved
…ric/CertifiedEventClient.scala
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Related Issues/PRs
#xxx
What changes are proposed in this pull request?
Scala client for certified events api hosted on ML workload Admin endpoint.
How is this patch tested?
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentation
folder.Make sure you choose the correct class
estimators/transformers
and namespace.DocTable
points to correct API link.yarn run start
to make sure the website renders correctly.<!--pytest-codeblocks:cont-->
before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTests
job pass in the pipeline.