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

D335: Open sourcing process: use ansys-api-edb #334

Merged
merged 14 commits into from
Dec 11, 2023

Conversation

SMoraisAnsys
Copy link
Contributor

@SMoraisAnsys SMoraisAnsys commented Nov 27, 2023

This PR includes changes to move into public. The most relevant one is the use of the ansys-api-edb package.

Changes:

  • remove proto files and use ansys-api-edb
  • update build dependencies to match ansys-api-edb
  • update project name and description
  • remove python3.7 from ci_cd.yml, tox.ini and README.rst
  • refact architecture to add core directory
  • update licence year
  • remove requirements files
  • add optional dependencies (notebook, tests, doc) in pyproject.toml file and update README.rst, tox.ini and ci_cd.yml accordingly
  • use PIP_EXTRA_INDEX_URL to leverage private pypi (where ansys-api-edb is available)
  • change project extra deps to be compatible with private pypi

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."""
Copy link
Contributor

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?

Copy link
Collaborator

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.

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 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",
Copy link
Contributor

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:

  1. before ansys-api-edb is published on pypi
  2. when working on local changes made to ansys-api-edb

Copy link
Contributor Author

@SMoraisAnsys SMoraisAnsys Nov 28, 2023

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 of pyedb-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 trigger pip install /path/to/ansys-api-edb yourself since AFAIK flit does not support specifying local dependencies. I think we could try to use poetry because, from my knowledge, this package would let us use local dependencies .Now, if you want to test pyedb-core, you can use your previous workflow (based on tox). There is a pip install step for tests, doc and notebooks. It might even be possible to modify the tox.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 ?

Copy link
Contributor

@hiro727 hiro727 Nov 28, 2023

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

Copy link
Contributor Author

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 !

@SMoraisAnsys SMoraisAnsys force-pushed the open_sourcing/ansys-api-edb branch from f7488ce to b0a9984 Compare November 28, 2023 12:12
Changes:
- sort imports
- format using black
- format to match flake8
- format some docstring to match pydocstyle
- trim trailing whitespace (and exclude files)
@SMoraisAnsys SMoraisAnsys force-pushed the open_sourcing/ansys-api-edb branch from b0a9984 to 7fcfb7d Compare November 28, 2023 12:18
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
@SMoraisAnsys SMoraisAnsys force-pushed the open_sourcing/ansys-api-edb branch from 5916969 to a7f1a00 Compare November 29, 2023 11:40
@hiro727 hiro727 changed the title Open sourcing process: use ansys-api-edb D335: Open sourcing process: use ansys-api-edb Nov 29, 2023
@SMoraisAnsys
Copy link
Contributor Author

Closes #335

@drewm102 drewm102 requested a review from bwnedrud December 7, 2023 17:04
@drewm102 drewm102 merged commit c0e3c29 into develop Dec 11, 2023
15 checks passed
@drewm102 drewm102 deleted the open_sourcing/ansys-api-edb branch December 11, 2023 18:27
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.

4 participants