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

Split web server into separate package #98

Merged
merged 19 commits into from
Oct 25, 2024
Merged

Split web server into separate package #98

merged 19 commits into from
Oct 25, 2024

Conversation

josephmckinsey
Copy link
Contributor

@josephmckinsey josephmckinsey commented May 24, 2024

  • Eliminated npm part in setup.py commands
  • Created separate package
  • rename to helics_cli_extras
  • move observer stuff to helics_cli_extras
  • move testing if necessary
  • setup ci
  • setup cd and PyPI package (🤞)
  • Changed CI to run on push to main or pull request to main (prevents annoying duplication)

- Eliminated npm part in setup.py commands
- Created separate package

TODO
- rename to helics_cli_extras
- move observer stuff to helics_cli_extras
- move testing if necessary
- setup ci/cd and PyPI package
@nightlark
Copy link
Member

nightlark commented May 24, 2024

Relates to #86

For configuring the CI/CD workflow for this with PyPI publishing, use:

Repository: [GMLC-TDC/pyhelics](https://github.com/GMLC-TDC/pyhelics)
Workflow: cd-cli-extras.yml
Environment name:        pypi-cli-extras

@nightlark
Copy link
Member

For backlog to make sdist eventually work: a custom in-source tree build backend interface that runs npm commands would likely be what we want.

@josephmckinsey josephmckinsey marked this pull request as ready for review June 7, 2024 17:54
@josephmckinsey
Copy link
Contributor Author

P.S. I had to remove the python package "install" that was being added to our python dependencies in the github actions workflow. I believe it was never intended to be installed.

Copy link
Member

@nightlark nightlark left a comment

Choose a reason for hiding this comment

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

It looks so much nicer not having all that code just to run npm in the setup.py -- only thing I'm not sure about is if we should have checks for those two imports to return a nicer error message if a user tries to run those functions without installing the helics-cli-extras.

helics/cli.py Outdated Show resolved Hide resolved
helics/cli.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@nightlark nightlark left a comment

Choose a reason for hiding this comment

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

I think the workflow changes look okay -- after merging try triggering the workflow to publish an initial version of the helics-cli-extras package.

@josephmckinsey josephmckinsey merged commit cfe4eb2 into main Oct 25, 2024
3 of 4 checks passed
@josephmckinsey josephmckinsey deleted the jm/split_web branch October 25, 2024 14:52
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