-
Notifications
You must be signed in to change notification settings - Fork 2
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
D335: Open sourcing process: use ansys-api-edb #334
Conversation
Changes: - use ansys.edb.core instead of ansys.edb - use ansys.api.edb instead of files from local protos
Changes: - remove all requirements files - add project.optional-dependencies : notebook, tests, doc - update README.rest, tox.ini and ci_cd.yml accordingly
@@ -0,0 +1,8 @@ | |||
"""This package contains code unavailable to API users.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the idea is to discorage users from using directly, I would prefer ansys.edb.core.internal
@drewm102 any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hiro727 @SMoraisAnsys Yes, I agree. I think using ansys.edb.core.internal
would be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do it to discourage users from using it. It's just a pattern used in other pyansys projects https://github.com/ansys/pymapdl/tree/main/src/ansys/mapdl/core, https://github.com/ansys/pyansys-geometry/tree/main/src/ansys/geometry/core, https://github.com/ansys/pyfluent/tree/main/src/ansys/fluent/core.
It should help to leverage common github action (if none exist atm, they might appear later on) and simplify common project maintenance.
dependencies = [ | ||
"importlib-metadata >=4.0", | ||
"protobuf>=3.20,<4", | ||
"ansys-api-edb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would this work in case of:
- before ansys-api-edb is published on pypi
- when working on local changes made to ansys-api-edb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. As stated in the PR note, this has side effects for the moment since ansys-api-edb
is not yet public.
Collaborators will need to have access to ansys-api-edb
. Here are cases I have in mind:
- Users have to get access to
ansys-api-edb
. Once access is granted, they have to clone the repo locally and install it locally. Then, the installation ofpyedb-core
is not a problem. - Developpers also need to have access to
ansys-api-edb
. If you want to work on local changes of ansys-api-edb, you'll have to triggerpip install /path/to/ansys-api-edb
yourself since AFAIKflit
does not support specifying local dependencies. I think we could try to usepoetry
because, from my knowledge, this package would let us use local dependencies .Now, if you want to testpyedb-core
, you can use your previous workflow (based on tox). There is apip install
step for tests, doc and notebooks. It might even be possible to modify thetox.ini
file to make it work with :
[testenv]
deps =
-e /path/to/your/local/ansys-api-edb
install_command =
...
- To use the CI, a temporary tar ball of
ansys-api-edb
could be uploaded there and used until the package gets public.
What are your thoughts on this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depends on the timeline to get ansys-api-edb published. broken unit tests would not be a big deal atm but broken doc build might pose some challenge in testing/getting documentation certified.
one option would be to include ansys-api-edb
as a submodule to this repo then ci can build api as part of this build process (including when local changes are made to api repo)
if that is not ideal, maybe we could post a release asset of api and download it from here during ci runs as you mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI should be fixed with a7f1a00
Could you please update the PR title to match your naming style ? This should make the PR pass the whole CI !
Changes: - use newer repo revisions - add common pyansys repo
Changes are to leverage common pyansys file directory structure
f7488ce
to
b0a9984
Compare
Changes: - sort imports - format using black - format to match flake8 - format some docstring to match pydocstyle - trim trailing whitespace (and exclude files)
b0a9984
to
7fcfb7d
Compare
The main issue we had was to retrieve `ansys-api-edb`. It is now accessible through private pypi. Changes: - use PIP_EXTRA_INDEX_URL to leverage private pypi - remove python3.7 from CI and tox.ini - fix doc-build deps - fix tox.ini by installing with .[tests] - change project extra deps to be compatible with private pypi
5916969
to
a7f1a00
Compare
Closes #335 |
This PR includes changes to move into public. The most relevant one is the use of the
ansys-api-edb
package.Changes:
ansys-api-edb
ansys-api-edb
core
directoryansys-api-edb
is available)