Skip to content

Conversation

@kdestin
Copy link
Member

@kdestin kdestin commented Feb 15, 2023

Description

Related to #28742
Related to #28782

This PR runs black on the azure-ai-ml in preparation for #28756 which is scheduled to be merged on 2023-02-15.

Methodology: black --config eng/black-pyproject.toml sdk/ml (eng/black-pyproject.toml was introduced in #28835 )

cc: @scbedd

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added ML-Assets ML-Inference AreaPath labels Feb 15, 2023
@kdestin
Copy link
Member Author

kdestin commented Feb 15, 2023

/azp run python - ml - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@kdestin kdestin force-pushed the run-black-on-ml branch 3 times, most recently from 9a48248 to f70b3c8 Compare February 15, 2023 10:52
@needuv
Copy link
Member

needuv commented Feb 15, 2023

PR looks good, but I'm wondering if we should skip _restclient? It's anyways autogenerated code, so I don't think we need to worry about formatting it. However, if the PR check doesn't allow for exclusions, then I guess we have no choice.

@kdestin
Copy link
Member Author

kdestin commented Feb 15, 2023

PR looks good, but I'm wondering if we should skip _restclient? It's anyways autogenerated code, so I don't think we need to worry about formatting it. However, if the PR check doesn't allow for exclusions, then I guess we have no choice.

Is there any harm in keeping _restclient included? There's a one-time cost every time the restclient is regenerated to run black, but otherwise it's not obvious to me what the negative is.


From what I've seen of the CI infrastructure for formatting, there isn't currently a mechanism for a package to specify formatting exclusions. So if we really wanted to exclude _restclient, this PR would be blocked until that's implemented.

@needuv
Copy link
Member

needuv commented Feb 15, 2023

Is there any harm in keeping _restclient included? There's a one-time cost every time the restclient is regenerated to run black, but otherwise it's not obvious to me what the negative is.

These days, our partners are usually the ones to onboard new API versions, and there tends to be quite a bit of churn in _restclient code until the API is finalized. I'm not sure if expecting them to run black every time they regenerate something is too much to ask, so maybe we should have a quick internal discussion about it. If we decide that _restclient will be included (since it was excluded from any formatting in the internal repo), we should make it clear in our documentation so that partners aren't confused about the change in rules.

@kdestin
Copy link
Member Author

kdestin commented Feb 15, 2023

Is there any harm in keeping _restclient included? There's a one-time cost every time the restclient is regenerated to run black, but otherwise it's not obvious to me what the negative is.

These days, our partners are usually the ones to onboard new API versions, and there tends to be quite a bit of churn in _restclient code until the API is finalized. I'm not sure if expecting them to run black every time they regenerate something is too much to ask, so maybe we should have a quick internal discussion about it. If we decide that _restclient will be included (since it was excluded from any formatting in the internal repo), we should make it clear in our documentation so that partners aren't confused about the change in rules.

Made a PR ( #28835 ) that should configure black to exclude the _restclient from formatting. That PR needs to be merged before this one, but I've already re-run black with those exemptions.

@needuv needuv merged commit 675e8a4 into Azure:main Feb 15, 2023
@kdestin kdestin mentioned this pull request Feb 23, 2023
6 tasks
@kdestin kdestin deleted the run-black-on-ml branch August 10, 2023 02:13
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