-
Notifications
You must be signed in to change notification settings - Fork 19
Add MCP server #39
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
Add MCP server #39
Conversation
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.
Absolutely fantastic work! 🚀
I put in some comments. Some are necessary, but a lot of them are nits. We can push a lot of these to the git issues and handle them from there post-merge.
| if not isinstance(body, dict): | ||
| raise ValueError( | ||
| "Create operations require a request body that matches the CreateTableRequest schema. See CreateTableRequest in " | ||
| "https://raw.githubusercontent.com/apache/polaris/apache-polaris-1.2.0-incubating/spec/generated/bundled-polaris-catalog-service.yaml" |
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.
We can file a GitHub issue here, but we are linking a specific version of the Apache Polaris interface. We could:
- Modify this to a redirect to the most recent version
- Have the user pass in an Polaris version
- Have some way to communicate which version Polaris is and have the MCP Server pull in the right version of the entities
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.
We will need to think through, as this is not the only place where MCP server is tied to particular Polaris server. A dynamic context switch for different Polaris server version may not be possible. I mentioned the version mapping in my another comments. WDYT?
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.
Right now, I'd recommend you:
- File a GitHub issue
- Link this comment
- Later, have a longer conversation on the Dev ML with multiple options. In my opinion, this is an "architecturally significant requirement," so there will be multiple perspectives. I don't want this sort of consensus building to block our users from trying out the first version, so I think we can defer this until later.
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.
Filed #50
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.
Thanks!
adam-christian-software
left a comment
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.
This PR looks good to me. I understand that we have a lot of filed issues to work on, but this looks good for a first version where we can iterate on. Thanks!
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.
This looks great! I left some comments from project management perspective but I do not think they should be blocker of this initial PR.
One thing we probably need is the github workflow to run unit tests
|
|
||
| [project] | ||
| name = "polaris-mcp" | ||
| version = "0.9.0" |
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.
Curious, what's the plan for the package version. Is 1.0.0 the next version same as what polaris previously did?
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.
I think so. 0.9.0 would server as a preview version before we move to 1.0.0. WDYT? cc @adam-christian-software
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.
So, I think there are two things here:
- The "how" we ship.
- The "versioning" heuristics we should use.
For #1, @flyrain has filed an Issue ( #46 ) to investigate the packaging and publishing. Right now, I don't believe that we need to think about this since we can largely borrow the principles that we use for the Python CLI when we have completed that work.
For #2, I have assumed that we will use the typical semantic versioning approach (https://semver.org/). While that helps us determine when we do breaking changes and when we don't, it does not provide the entire guidance. SemVer states:
- Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
- Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.
For this particular project, I would say that it makes sense for us to release first with 0.9.0 because this work is still very much in a prototype phase. We have several Git issues to tackle to firm up the solution. We should get those done after this PR and a lot of those items should be done before the first release.
This begs the question: When do we go from 0.X.0 to 1.0.0? For me, I would say a few things should happen:
- We have one user who has used this work and given feedback.
- We should not have any critical issues.
- We should have good-enough unit & IT testing.
- As this is a client of our Polaris server, we should nail down our compatibility story.
- We should do a quick audit of any security issues.
- We should have user-facing documentation on the Polaris Hugo site.
- We should have all of the operational readiness items done - deployment processes, monitoring/observability, & upgrades/rollback processes.
Until we nail those 7 things, I wouldn't put this as a 1.0.0 project.
| "fastmcp>=2.13.0.2", | ||
| "urllib3>=1.25.3,<3.0.0", | ||
| ] | ||
|
|
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.
It will be good to add format and lint tools to this project, as those make collaboration easier.
precommit + ruff would be a good option: https://github.com/apache/polaris/blob/main/client/python/.pre-commit-config.yaml
We could create an issue and add that in a follow-up
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.
Added. Thanks for the suggestion!
|
Python github CI works well: |
Thanks a lot for the review, @HonahX ! Added github CI for lint and unit tests. Please take another look. |
HonahX
left a comment
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.
LGTM!
This PR is moved from apache/polaris#2980, per discussion here, https://lists.apache.org/thread/8w5n089mtsyd3mvv7j89h0h7nvvp7o4w.
cc @adam-christian-software