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

add poetry support #624

Closed
wants to merge 4 commits into from
Closed

add poetry support #624

wants to merge 4 commits into from

Conversation

an-li-the-dev
Copy link
Contributor

@an-li-the-dev an-li-the-dev commented Dec 16, 2022

Changes:

  • Poetry adoption
  • release.yml workflow change in poetry way(not tested)
  • tidy3d configure cli for apikey setup
    • install in from local codebase: poetry install
  • Add apikey as new authentication way, auth order:
    • SIMCLOUD_APIKEY environment variable
    • ~/.tidy3d/config for api key
    • credential

@an-li-the-dev an-li-the-dev force-pushed the feature/poetry branch 21 times, most recently from c1a7c44 to 558fd07 Compare December 19, 2022 09:15
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Hey @An-Li-magicloud just a comment: I noticed that tox.ini was removed and now the tests just run pytest. Is there any way to keep tox? It is very useful for us to test many python versions 3.7+, OS, and configure things like environment variables, which we need for some features coming soon. If there is a way to keep it, that would be ideal. This article describes a way to do it link.
tox.ini

[tox]
skipsdist = true
envlist = py37, py38

[testenv]
whitelist_externals = poetry
commands =
    poetry install -v
    poetry run pytest

@an-li-the-dev
Copy link
Contributor Author

an-li-the-dev commented Dec 20, 2022

Sure, I can revert tox back.

The reason I remove tox because it's configuration was incorrect: github action create environments per python version, but tox spawns the same python matrix inside of each environment.

You can see the elapsed time differences from success task:
screenshot-20221220-110603

I will add it back and correct the config as well.

@an-li-the-dev an-li-the-dev force-pushed the feature/poetry branch 7 times, most recently from e7b7724 to ae3917e Compare December 20, 2022 07:17
@tylerflex tylerflex changed the base branch from feature/apikey to pre/1.9.0 January 10, 2023 21:05
@an-li-the-dev an-li-the-dev force-pushed the feature/poetry branch 4 times, most recently from 7ce3ac7 to 2de2127 Compare January 19, 2023 03:40
@tylerflex
Copy link
Collaborator

I'm reluctant to merge this because more and more we are relying on dependency groups. For example, we have added [jax] as a group and perhaps soon [gdstk]. If we can't figure out a good solution to this, I think we should stick with the setup.py solution as the main point of using poetry initially was for the CLI, right?

@momchil-flex momchil-flex force-pushed the pre/1.9.0 branch 2 times, most recently from 7acc7b1 to d58051e Compare February 14, 2023 18:18
@an-li-the-dev
Copy link
Contributor Author

an-li-the-dev commented Mar 3, 2023

Sorry for the late response. Here is another solution by using extras, here is an example to make gdstk optional:

[tool.poetry.dependencies]
gdstk = { version = "xxx", optional=true }

[tool.poetry.extras]
gds = ["gdstk"]

When user needs gdstk in there local environment, use -E to declare deps.

poetry install -E gds
# or
pip install tidy3d[gds]

https://python-poetry.org/docs/pyproject/#extras

  • basic.txt goes into [tool.poetry,dependencies] without optional
  • core.txt is basic + web, since basic already satisfied, we can rename "web" to "core"
  • for dev, user could run poetry install --all-extras to install all deps.
  • extras doesn't support inheritance which mean deps declaration maybe redundant, but it can find-granted.

@tylerflex
Copy link
Collaborator

Thanks for looking into this An, I'm planning to merge the webapi refactor next (#730) and then revisiting this once that is in.

I think for me it seems we can do everything we currently need already with setup.py. If we want to switch to poetry, it will involve some learning curve and adjustment to the way we do things, including on the backend, so I think we want to make sure it is worth the investment if we do change, especially now that we are trying to finish a few things before a major release.

In your opinion, what would be the main advantage(s) of moving to poetry? Do you think it's worth it for the future of Tidy3D?

@an-li-the-dev
Copy link
Contributor Author

an-li-the-dev commented Mar 6, 2023

Here are what I felt poetry is better than raw pip:

  • virtual env integrated
  • Unified environment -- we want to solve dependencies once, then run it in every environments, include our local development, user's local machine and servers.
  • Security/deprecated upgrade. Poetry could fine-grained manage dependencies, it frees users from upgrading hell when they use legacy python versions, eg. 3.6, 3.7, and pynum and it's related packages compliant each other.
matplotlib = [{ python = "~3.7", version = "~3.5" }, { python = "^3.8", version = "^3.6" }]
pandas = [{ python = "~3.7", version = "^1.3.0"}, {python = "^3.8", version = "^1.5.0"}]

@tylerflex tylerflex changed the base branch from pre/1.9.0 to pre/1.10.0 March 7, 2023 20:45
@tylerflex tylerflex added the WIP label Mar 8, 2023
@tylerflex
Copy link
Collaborator

I will close this PR for now because it's diverged so much from the main branch. But when (and if) we do poetry, I will pull aspects from this PR and maybe make a new one.

@tylerflex tylerflex closed this Mar 27, 2023
@tylerflex tylerflex deleted the feature/poetry branch May 16, 2023 14:18
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

3 participants