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

[KED-3060] Create viz version graphql endpoint #727

Merged
merged 13 commits into from
Feb 3, 2022

Conversation

tynandebold
Copy link
Member

@tynandebold tynandebold commented Feb 2, 2022

Description

For an upcoming frontend feature, we need a way to programmatically check the user's installed Viz version and the latest one released on PyPI.

Development notes

I've created a GraphQL query (version) that returns the current and latest versions.

QA notes

To test:

  1. Check out the branch
  2. Run make run
  3. Open http://127.0.0.1:4142/graphql
  4. Run this query:
query MyQuery {
  version {
    installed
    latest
  }
}

The output should look akin to this:

{
  "data": {
    "version": {
      "installed": "4.3.1",
      "latest": "4.3.1"
    }
  }
}

P.S. this is my first Python PR. Please be critical of my code and help me learn :)

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Tynan DeBold <[email protected]>
@limdauto
Copy link
Collaborator

limdauto commented Feb 2, 2022

Which screen are we doing this on? If we do it on the flowchart, we might need a rest endpoint. Otherwise we will need to port Apollo client to the flowchart screen, which is actually a good thing but might increase the scope of the ticket.

@tynandebold
Copy link
Member Author

Which screen are we doing this on? If we do it on the flowchart, we might need a rest endpoint. Otherwise we will need to port Apollo client to the flowchart screen, which is actually a good thing but might increase the scope of the ticket.

It's mocked up on the flowchart but I think it should exist on both routes, flowchart and experiment tracking.

Why would we need a rest endpoint? Our Apollo <Provider /> wraps the whole app, not just the experiment tracking part. Or is it for another reason?

@tynandebold tynandebold requested a review from limdauto February 2, 2022 14:18
…ature/KED-3060-create-viz-version-endpoint

Signed-off-by: Tynan DeBold <[email protected]>
@limdauto
Copy link
Collaborator

limdauto commented Feb 2, 2022

@tynandebold ah I see. I was under the impression that the Apollo Provider was on the exp route only. Don't mind me then. This is actually fantastic news.

Copy link
Collaborator

@limdauto limdauto left a comment

Choose a reason for hiding this comment

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

Nice one!

Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
@tynandebold tynandebold requested a review from yetudada as a code owner February 2, 2022 14:47
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Look's great Tynan! :)
i suppose we will just need to update the fe schema and add the tests !

@studioswong
Copy link
Contributor

@tynandebold ah I see. I was under the impression that the Apollo Provider was on the exp route only. Don't mind me then. This is actually fantastic news.

Yep @limdauto the idea behind wrapping the ApolloProvider on app level rather than on a route is to allow the gradual migration of all endpoints to graphql, with new needs on data to be built out as new graphql queries while slowly stripping away the need for rest in stages ( I am working on a spike doc on the next steps for that as we speak - will share with you once it's drafted. )

@@ -49,3 +50,8 @@ union UpdateRunDetailsResponse = UpdateRunDetailsSuccess | UpdateRunDetailsFailu
type UpdateRunDetailsSuccess {
run: Run!
}

type Version {
installed: String!
Copy link
Contributor

Choose a reason for hiding this comment

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

naming this as Current might be more relevant as I understand there can be multiple versions of a package installed in a python environment ( @limdauto @AntonyMilneQB can correct me if I'm wrong)

Copy link
Collaborator

Choose a reason for hiding this comment

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

there can't be multiple versions of a package installed in the same python environment. There can be many environments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @limdauto - on that note, I've heard that it is possible to install multiple versions of a package by specifying it to be installed in a different directory other than the default site-packages? ( not that it is a common use case at all) Again, I'm no python dev, so you know best 😄

On the subject, Current version to me sounds more intuitive as we normally refer to 'current' and 'latest' versions - curious to hear your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I didn't like current is because I didn't know what it was describing, and it seemed like it could describe both. As in: is it the current version the user has installed? Or is it the current version that's released on PyPI?

With installed and latest I think it's more clear. It might even be clearer if it's installed and latestRelease.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't like current for the same reason as @tynandebold mentioned. installed and latest make most sense to me.

With that said, we are already using current as the variable name where we do the CLI nudge. So whatever we go for, let's be consistent so we don't have current, latest and installed in the codebase.

Copy link
Contributor

@antonymilne antonymilne Feb 3, 2022

Choose a reason for hiding this comment

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

@studioswong you could indeed install different version of the same Python package to different locations. But when we do from kedro_viz import __version__, it will be well-defined and import from only one installation of the package (the precedence of different locations is given by sys.path in Python). So it does make sense to refer to the "installed Python package" as a unique entity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sticking with installed and latest. I'll change in the cli files, too.

Copy link
Contributor

@studioswong studioswong left a comment

Choose a reason for hiding this comment

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

just tested and it works well, nice one.

As @rashidakanchwala mentioned, please add the relevant python tests for this endpoint.

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Awesome work, and top-notch Pythoning 🐍

I have some minor comments and questions - easier to discuss over a call, but just to list them:

  • pedantic suggestions for get_version()
  • when does this get called?
  • question about the schema types and whether some fields are nullable
  • latest < installed logic
  • apollo schema (just for my understanding)

Signed-off-by: Tynan DeBold <[email protected]>
@tynandebold tynandebold removed the request for review from yetudada February 3, 2022 12:45
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Looks great Tynan!!! thanks

Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
@tynandebold tynandebold merged commit 7cae9c8 into main Feb 3, 2022
@tynandebold tynandebold deleted the feature/KED-3060-create-viz-version-endpoint branch February 3, 2022 16:48
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.

5 participants