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

Expose the API key/host/secret as variables for the users #29

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

wvangeit
Copy link
Contributor

Allows users to use
OSPARC_API_HOST / OSPARC_API_KEY / OSPARC_API_SECRET
in notebooks to access the API.

@wvangeit
Copy link
Contributor Author

FYI @pcrespov

Related to ITISFoundation/osparc-simcore#5695 (review)

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx

Copy link
Collaborator

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Please make sure OSPARC_API_HOST si resolvable on all deployments before this change goes in

image: $${SIMCORE_REGISTRY}/simcore/services/dynamic/jupyter-math:$${SERVICE_VERSION}
environment:
- OSPARC_API_HOST=$${OSPARC_VARIABLE_API_HOST}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not exist in Osparc codebase ad it will not resolve on all deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean this is because the API server is not available on some deployments? Or just because the variable is not set yet?

@pcrespov in your new PR on simcore, what will happen if there is no API server is available? Will this var be empty/None or so, or will it just not be set?

Copy link
Member

@pcrespov pcrespov Apr 19, 2024

Choose a reason for hiding this comment

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

@GitHK Actually, waiting for you review ;-) ITISFoundation/osparc-simcore#5695

Copy link
Member

Choose a reason for hiding this comment

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

@wvangeit i will then pull it and use it locally.

Copy link
Member

@pcrespov pcrespov Apr 19, 2024

Choose a reason for hiding this comment

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

what will happen if there is no API server is available? Will this var be empty/None or so, or will it just not be set?

@wvangeit Good question. In this case, I can see in the code that it simply does not get replaced.

https://github.com/itisfoundation/osparc-simcore/blob/5035fe5cc7a7d7ee9fcab15fd7f8a116a4934cd6/services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/docker_compose_specs.py#L340-L349

Apparently the services will fail upon start if OSPARC_VARIABLE_API_HOST is not exposed in osparc. IMO it is a bit extreme since a normal user (i.e. somebody that is not the service owner) will not understand why this service failed to start. If this expression is an an .env file and OSPARC_VARIABLE_API_HOST is undefined it then OSPARC_API_HOST gets assigned an empty string! Nonetheless @GitHK, hwo is the code owner, has a different view. He will write it down later for you to be aware of it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed your PR. Got distracted. It's all good 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.

I would vote for empty string. Then the user and the python api client can catch this, and notice that the api host is undefined. (in a sense some 'null' instead of empty string would be better, but not sure if there is a stable null equivalent in the world of env vars)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer that OSPARC_VARIABLE_* have to be resolvable in order for the service to start. Unlike the OSPARC_VARIABLE_VENDOR_SECRET_*, which are define din the db, these are provided always by the platform.

If you are requesting an OSPARC_VARIABLE_*, which does not exist, this is a clear way to signal that you did something wrong.

Also I think it's the user's responsibility to make sure that the service stars once they make changes to it. He can always test it in a local deployment before publishing it elsewhere. Unfortunately we don't have a better workflow for this part.

Copy link
Member

Choose a reason for hiding this comment

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

  • FYI vendor secrets are also provided by the service owner.
  • directorv2 also fails to start service when the vendor secrets is not available.

@pcrespov pcrespov requested a review from GitHK April 19, 2024 09:27
image: $${SIMCORE_REGISTRY}/simcore/services/dynamic/jupyter-math:$${SERVICE_VERSION}
environment:
- OSPARC_API_HOST=$${OSPARC_VARIABLE_API_HOST}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed your PR. Got distracted. It's all good now.

environment:
- OSPARC_API_HOST=$${OSPARC_VARIABLE_API_HOST}
- OSPARC_API_KEY=$${OSPARC_VARIABLE_API_KEY}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very useful feature in the related PR, thanks @pcrespov. If I get this right, if the services specs are adapted correctly, users will not need anymore to create API keys and secrets when they run from inside oSPARC!

I am not sure what the conclusion about OSPARC_API_HOST. I think the obvious thing to avoid is that the service fails to start if this is not set. 😉

Small question: ${OSPARC_VARIABLE_API_HOST} and ${OSPARC_VARIABLE_API_KEY} will always be available, right?

Copy link
Member

Choose a reason for hiding this comment

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

@elisabettai yes, they are.
SEE also ITISFoundation/osparc-simcore-clients#149

@elisabettai can you please merge these changes and produce a new version of this service and inject in master registry so we can test it there?. thx

Copy link
Collaborator

@elisabettai elisabettai Apr 22, 2024

Choose a reason for hiding this comment

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

To (auto-magically) publish a new version of this: @wvangeit, can you please run make version-patch? Then I'll merge and take care of publishing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elisabettai could you also deploy it to osparc.io now.
Metamodelling is working there now, I need this variable accessible in the notebooks to test the axondeepseg metamodelling study. Thx.

@wvangeit
Copy link
Contributor Author

@elisabettai , I ran the version-patch. Thx

@elisabettai elisabettai merged commit e61edae into ITISFoundation:main Apr 22, 2024
1 check passed
@elisabettai
Copy link
Collaborator

@pcrespov, @wvangeit, the new version (3.0.2) of JupyterLab Math is now available on osparc-master (and s4l-master).

Let me know if it works as expected and if you need it on other deployments (e.g. osparc.io). jfyi, we haven't published the 3.x.x on osparc.io yet, since we wanted to test the inactivity monitoring in master before publishing it to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants