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

adapt to AWS lambda environment #1100

Merged
merged 1 commit into from
Aug 29, 2023
Merged

adapt to AWS lambda environment #1100

merged 1 commit into from
Aug 29, 2023

Conversation

magiWei
Copy link

@magiWei magiWei commented Aug 25, 2023

No description provided.

@magiWei magiWei force-pushed the qingeng/aws-lambda branch 2 times, most recently from 82b1d07 to 621fdb9 Compare August 25, 2023 13:31
@momchil-flex
Copy link
Collaborator

momchil-flex commented Aug 25, 2023

Changes for hdf5 handling look good. Please revert all changes related to the config file, they are not needed and are also making the tests fail (can't import from tidy3d in setup.py).

I will test once these changes are reverted and merge if all good.

@magiWei
Copy link
Author

magiWei commented Aug 25, 2023

config file

config file? which one?

@momchil-flex
Copy link
Collaborator

config file

config file? which one?

Revert all the changes related to ~/.tidy3d folder

@magiWei
Copy link
Author

magiWei commented Aug 25, 2023

~/.tidy3d

so, if user cannot write ~/.tidy3d, they use os.enviroment?

@momchil-flex
Copy link
Collaborator

momchil-flex commented Aug 25, 2023

Yes.

However, this makes me realize something: when we had user name and password authentication, the user was asked whether they want to store credentials to file.

Now it seems like if user does configure, the API key is always written to file. This would error on lambda. But shouldn't we still ask user such that they can say no if they don't want the api key stored?

@lei-flex what do you think?

@magiWei
Copy link
Author

magiWei commented Aug 25, 2023

Yes.

However, this makes me realize something: when we had user name and password authentication, the user was asked whether they want to store credentials to file.

Now it seems like if user does configure, the API key is always written to file. This would error on lambda. But shouldn't we still ask user such that they can say no if they don't want the api key stored?

@lei-flex what do you think?

This is my concern, if the user needs to use username/pwd, f"{expanduser('~')}/.tidy3d" can't be writted on lambda, so I add one "/tmp/.tidy3d"

TIDY3D_DIR = f"{expanduser('~')}/.tidy3d"
TIDY3D_DIR2 = "/tmp/.tidy3d"
if not os.access(TIDY3D_DIR, os.W_OK):
TIDY3D_DIR = TIDY3D_DIR2

@momchil-flex
Copy link
Collaborator

TIDY3D_DIR = f"{expanduser('~')}/.tidy3d"
TIDY3D_DIR2 = "/tmp/.tidy3d"
if not os.access(TIDY3D_DIR, os.W_OK):
TIDY3D_DIR = TIDY3D_DIR2

Ok let's keep this part as long as you don't have to import anything in setup.py.

@momchil-flex
Copy link
Collaborator

Note tests are failing currently.

@magiWei magiWei force-pushed the qingeng/aws-lambda branch 2 times, most recently from 0166f51 to a056231 Compare August 26, 2023 00:07
@magiWei
Copy link
Author

magiWei commented Aug 26, 2023

Note tests are failing currently.

tests are ok now, Do we need add one test about lambda function.

@momchil-flex
Copy link
Collaborator

How can we test it? If possible to mock then yeah why not.

@magiWei
Copy link
Author

magiWei commented Aug 26, 2023

How can we test it? If possible to mock then yeah why not.

I test it on https://github.com/flexcompute/tidy3d-python-webapi, add one new release-xxx and write one api, then deploy to lambda,

@magiWei magiWei force-pushed the qingeng/aws-lambda branch 6 times, most recently from 039454b to ab29beb Compare August 28, 2023 08:27
@momchil-flex momchil-flex merged commit 7212126 into pre/2.4 Aug 29, 2023
16 checks passed
@momchil-flex momchil-flex deleted the qingeng/aws-lambda branch August 29, 2023 23:37
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.

None yet

2 participants