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

feat: update python api client #193

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: update python api client #193

wants to merge 12 commits into from

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Nov 4, 2024

Description

This PR updates the python API client to the most recently available api endpoints. I'm not 100% sure if I miss some decorators for @_requires_api_version('>=X.Y.Z') or @deprecated or if we can even fully remove the deprecated endpoint, but appreciate if you give this a thought yourself. Thanks

Review guidelines

Estimated Time of Review: 10 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

I think it would make sense to release a major update to this client after the v4 core release given the changes we made to revised /version and /protocol endpoints.

We could potentially remove the deprecated functions that are marked iwth removed_in='3.0.0' as part of the major update, but ones that haven't yet had a deprecated warning should get a new one before being removed entirely, just to try to keep the best user experience.

Some info and version compatibility table in the README also needs updating

python/src/aica_api/client.py Outdated Show resolved Hide resolved
python/src/aica_api/client.py Show resolved Hide resolved
@domire8
Copy link
Member Author

domire8 commented Nov 4, 2024

Some info and version compatibility table in the README also needs updating

Will do, I needed to make sure that I got all the code changes correctly first

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

From commit e3eaa7a I see you had considered renaming the static protected method _requires_api_version to _requires_core_version - I think you could safely do that as long as you fully update all references

@domire8
Copy link
Member Author

domire8 commented Nov 4, 2024

Let me also test this locally before we merge it

@domire8
Copy link
Member Author

domire8 commented Nov 4, 2024

Now I think we are good to go

@domire8 domire8 linked an issue Nov 11, 2024 that may be closed by this pull request
@domire8
Copy link
Member Author

domire8 commented Nov 15, 2024

Ok looks good to me 👍

@domire8
Copy link
Member Author

domire8 commented Nov 18, 2024

So currently,

aica.api_version()
# >>> Error getting version details! Expected a map of `signed_packages` to include `aica_api_server`: 'aica_api_server'

aica.core_version()
# >>> {'message': 'b\'{"online":true,"signed_packages":{},"entitlements":["aica_studio_access","is_aica_developer"]}\'. You have requested this URI [/api/version] but did you mean /api//version or /api/swagger.json or /api//v2/features ?'}

Are those results expected and desired? Especially the second one seems weird to me

EDIT: these results are due to the fact that I was using a custom core, i guess. With the v4.1.0 core version i get

>>> aica.core_version() 
'4.1.0'
>>> aica.api_version()  
'4.1.1'

which makes sense, but I'm still wondering why we get You have requested this URI [/api/version] but did you mean /api//version or /api/swagger.json or /api//v2/features ?

EDIT 2:
if aica.core_version() returns a weird dict there if the core is not signed, it basically means that the api client is not usable with custom cores because the _requires_core_version decorator complains obviously...It needs a string input, not a dict

@LouisBrunner
Copy link
Contributor

I am unsure about the URL "parsing" that is happening with startswith("http://") and such. What is this about?

python/CHANGELOG.md Outdated Show resolved Hide resolved
@eeberhard
Copy link
Member

I am unsure about the URL "parsing" that is happening with startswith("http://") and such. What is this about?

If we simply use an address like localhost (current default) or 127.0.0.1, the actual URL must be prepended with http://, otherwise we get a requests.exceptions.RequestException. But I also wanted to avoid prepending the http if the provided URL already contains it.

it basically means that the api client is not usable with custom cores

While this client is intended for official releases, it would of course be nice to use it with internal pre-release builds that don't have valid package signatures. I'll add a check for that case to bypass the version requirement with a warning

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

For me this is good to go now. Sorry for the long delay and many additional changes but it's quite a good improvement!

@@ -2,6 +2,7 @@

Release Versions:

- [3.1.0](#300)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [3.1.0](#300)
- [3.0.0](#300)

python/README.md Outdated
Use the following compatability table to determine which client version to use.

| API server version | Matching Python client version |
| AICA Core version | Matching Python client version |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| AICA Core version | Matching Python client version |
| AICA Core version | Matching Python client version |

@domire8
Copy link
Member Author

domire8 commented Dec 4, 2024

Still a few problems, are we happy with that?
image

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.

(API client) helper functions to manage and monitor sequence states
3 participants