Skip to content

Conversation

@mirdaki
Copy link
Contributor

@mirdaki mirdaki commented Jan 13, 2020

This is a new extension (in preview) to use the Aladdin service to power all examples in help content. It is pending on this PR being merged into the core CLI to work properly.


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

@azuresdkci
Copy link

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/mirdaki/azure-cli-extensions.git@ai-examples#subdirectory=src/$EXT&egg=$EXT"

@mirdaki mirdaki changed the title AI examples Add AI examples extension Jan 13, 2020
@mirdaki mirdaki requested a review from zikalino as a code owner January 13, 2020 23:12
@yonzhan yonzhan added this to the S164 milestone Jan 13, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Jan 13, 2020

add to S164.

src/index.json Outdated
"run_requires": [
{
"requires": [
"azure-cli-core"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to explicitly to specify azure-cli-core I think, azure-cli-extensions depends on azure-cli, and azure-cli depends on azure-cli-core.

You can have a try cause it failed CI.

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, removing the azure-cli-core dependency.

]

DEPENDENCIES = [
'azure-cli-core'
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to explicitly to specify azure-cli-core I think, azure-cli-extensions depends on azure-cli, and azure-cli depends on azure-cli-core.

You can have a try cause it failed CI.

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, removing the azure-cli-core dependency.

Copy link
Contributor

@haroldrandom haroldrandom left a comment

Choose a reason for hiding this comment

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

Please take a look comments and the reason why failed CI.

Please don't forget to update sha256Digest value once code change.

The next version of the CLI (presumably 2.0.81), will be needed for the
extension to work. However, it seems that some of the build issues might
be related to that version not existing. So to pass the CI/CD testing
(and to check that there are no other issues), the extension min version
has been set to the original default of 2.0.65.
@mirdaki
Copy link
Contributor Author

mirdaki commented Jan 15, 2020

OK, made the requested changes.

One other thing that might be causing the problems is that this extension needs some code that hasn't yet shipped in the Azure CLI. I had the min version set to be 2.0.81 (presumably the next version), but as that doesn't yet exist, it might of caused an issue. For now I've reverted it to 2.0.65, but that needs to be updated before this PR gets merged.

{
"azext.isPreview": true,
"azext.minCliCoreVersion": "2.0.65",
"azext.maxCliCoreVersion": "2.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this constraint if you are not sure about the max version requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good suggestion!

src/index.json Outdated
],
"ai-examples": [
{
"downloadUrl": "https://aladdinprodwestus.blob.core.windows.net/aladdin-extensions/ai_examples-0.1.0-py2.py3-none-any.whl",
Copy link
Contributor

Choose a reason for hiding this comment

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

download link should be generated by CLI team, and you don't need to provide it.

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, I was unclear how that was supposed to happen. I'll remove it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err, how am I supposed to update the index.json then? I had been using the azdev extension publish command to do that up till now, but if y'all will be publishing it, that doesn't make sense to use. The azdev extension update-index command looks like the right thing, but it also needs a URL to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Juliehzl any update on this?

Copy link
Member

@jsntcy jsntcy Feb 21, 2020

Choose a reason for hiding this comment

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

@mirdaki,

Put whl file in CLI storage or your storage?
If no special reasons, usually we recommend to put whl file in our (CLI) storage as most of extensions do now. Also it's beneficial for our future release pipeline automation as pipeline automation has no access to your storage.

currently the workflow is:

  • You build a whl file
  • You send whl file to us
  • We'll upload your whl file to our storage (as you don't have permission to upload to our storage)
  • We tell you the downloadUrl
  • You update the index.json with the downloadUrl we provide to you

In the future (one or two sprints later),
All these manual steps will be automated in our extension release pipeline.


# Replacements for core functions
def provide_examples(help_file):
return replace_examples(help_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we provide two options for users to select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is an experiment, we were planning on getting feedback for one scenario at a time. The append code is there, because that was the original plan, but we've decided to have just the Aladdin examples to better test them.

'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.4',
'Programming Language :: Python :: 3.5',
'Programming Language :: Python :: 3.6',
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't support 3.7 and 3.8?

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 was using the what the boilerplate from azdev provided, will update.

'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.4',
Copy link
Contributor

Choose a reason for hiding this comment

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

3.4 is not supported at azure-cli-core

Copy link
Contributor Author

@mirdaki mirdaki Jan 21, 2020

Choose a reason for hiding this comment

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

I was using the what the boilerplate from azdev provided, will update.

@mirdaki
Copy link
Contributor Author

mirdaki commented Jan 22, 2020

I've pushed an update that address some of the comments. The sha has not been updated yet, because I'm still waiting for an answer from @Juliehzl about how to properly set the downloadUrl and update the index.json.

@yonzhan
Copy link
Collaborator

yonzhan commented Jan 23, 2020

@Juliehzl please help with the above comment.

@mirdaki
Copy link
Contributor Author

mirdaki commented Feb 10, 2020

@Juliehzl and @yonzhan, I'm still waiting on an answer on how I'm suppose to publish the extension.

If the CLI team is supposed to supply the URL, does that also mean the CLI team will upload the extension to their own blob storage? In that case is the PR done and y'all take care of the rest?

If not, am I supposed to continue publishing the extention to our own blob storage using azdev extension publish? Or is it something else I need to do?

@Juliehzl
Copy link
Contributor

Juliehzl commented Feb 11, 2020

Hi @mirdaki, when the code is ready to merge, I can help you publish the extension if you want to store your extension wheel in cli storage account.
In addition, you publish it to your own storage account with --storage-account parameter. But you need to update index.json with download link.
Please let me know if you hope we help you do the publish.

@mirdaki
Copy link
Contributor Author

mirdaki commented Feb 11, 2020

@Juliehzl I've updated index.json to use the correct metadata and point to the updated blob storage. I am ready for the extension wheel to be uploaded to the CLI storage account and merge this PR.

@yonzhan yonzhan modified the milestones: S164, S165 Feb 12, 2020
@yonzhan yonzhan modified the milestones: S165, S166 Feb 15, 2020
@mirdaki
Copy link
Contributor Author

mirdaki commented Feb 24, 2020

@haroldrandom and @Juliehzl, I've switched the downloadUrl to the CLI storage and am ready for the code to be merged.

@haroldrandom haroldrandom merged commit 6629998 into Azure:master Feb 25, 2020
ManuInNZ pushed a commit to ManuInNZ/azure-cli-extensions that referenced this pull request Apr 11, 2020
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.

8 participants