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: q3as in default use case template #105

Merged
merged 6 commits into from
Dec 7, 2024

Conversation

Angel-Dijoux
Copy link
Contributor

No description provided.

@Angel-Dijoux Angel-Dijoux self-assigned this Nov 19, 2024
Copy link
Contributor

@jpopesculian jpopesculian left a comment

Choose a reason for hiding this comment

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

I like this idea of adding an extra notebook. I would probably rename it to q3as_demo.ipynb and just add some info to the README.md for what that file is. I probably wouldn't link it as a solution ref in the pyproject.toml because there's no input and output in there and that would probably mess people up, but having it as an extra notebook as an example is cool and they can just copy the relevant cells if they do want to use it

"Start by visiting https://q3as.aqora.io and signing in with your GitHub or Google account. Click on your profile in the top right and go to API Keys. Tap Add API Key and enter a description for your API key. Tap Copy JSON to clipboard and paste the result in a file on your compute\n",
"\n",
"\n",
"> ⚠️ **Warning: Be Cautious with Secrets!**\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, this is a good point. unfortunately because the way that notebooks and the translation to scripts works, using things like importlib.resources is usually the best bet, but it requires that the file be in the package which means it would get uploaded. to handle this correctly, I would say we should make passing the credentials by environment variables possible which would require modifying the q3as client i believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be really cool too if we use environment variables, is to automatically add q3as credentials to aqora with aqora q3as add-key <PUBLIC> <SECRET> and then have aqora inject it directly into the virtual environment paths when doing aqora install / test so the user just has to set it up once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, this is a good point. unfortunately because the way that notebooks and the translation to scripts works, using things like importlib.resources is usually the best bet, but it requires that the file be in the package which means it would get uploaded. to handle this correctly, I would say we should make passing the credentials by environment variables possible which would require modifying the q3as client i believe.

But for now, the user doesn't send a file. Instead, I ask them to enter their credentials in the notebook. Shouldn't we handle it differently?

What would be really cool too if we use environment variables, is to automatically add q3as credentials to aqora with aqora q3as add-key <PUBLIC> <SECRET> and then have aqora inject it directly into the virtual environment paths when doing aqora install / test so the user just has to set it up once

Maybe is another story if that work for you?

Copy link
Contributor

@jpopesculian jpopesculian left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just the one thing

"source": [
"from q3as import Client, Credentials\n",
"\n",
"id = \"your-id\"\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just change this to import from the environment as a good standard practice for them not to include sensitive data and we can update the SDK to support it in the future. maybe even use python-dotenv?

@Angel-Dijoux Angel-Dijoux force-pushed the angel/add_q3as_use_case_template branch from f83b4b9 to 2f889d2 Compare November 25, 2024 20:29
Copy link
Contributor

@jpopesculian jpopesculian left a comment

Choose a reason for hiding this comment

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

One last thing: I'd include a .env file at the root of the template with

# Copy your API key ID and secret into the following fields
Q3AS_ID = "<Your Q3AS API key ID>"
Q3AS_SECRET = "<Your Q3AS API key secret>"

"\n",
"To run your algorithms in the cloud, you have to create an API key and load it into your Credentials\n",
"\n",
"Start by visiting https://q3as.aqora.io and signing in with your GitHub or Google account. Click on your profile in the top right and go to API Keys. Tap Add API Key and enter a description for your API key. Tap Copy id and secret and paste them in the notebook. \n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to say paste into the .env

"Start by visiting https://q3as.aqora.io and signing in with your GitHub or Google account. Click on your profile in the top right and go to API Keys. Tap Add API Key and enter a description for your API key. Tap Copy id and secret and paste them in the notebook. \n",
"\n",
"\n",
"> ⚠️ **Warning: Be Cautious with Secrets!**\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove this? It seems unnecessary and scary

Copy link
Contributor Author

@Angel-Dijoux Angel-Dijoux Nov 30, 2024

Choose a reason for hiding this comment

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

Since you recommend using a .env file, I wouldn't remove it. I mean, sometimes users might accidentally push credentials! Well, it's fine, I think. I ended up removing it!

@Angel-Dijoux Angel-Dijoux force-pushed the angel/add_q3as_use_case_template branch from 4b905de to f090db1 Compare December 5, 2024 19:05
@jpopesculian jpopesculian force-pushed the angel/add_q3as_use_case_template branch from f090db1 to 2606a8c Compare December 7, 2024 10:26
@jpopesculian jpopesculian force-pushed the angel/add_q3as_use_case_template branch from 686e2a0 to 6b0edae Compare December 7, 2024 10:54
@jpopesculian jpopesculian merged commit e2203c8 into main Dec 7, 2024
9 checks passed
@jpopesculian jpopesculian deleted the angel/add_q3as_use_case_template branch December 7, 2024 10:57
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.

2 participants