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

Simplified pip and conda environment; pypi deployment #181

Merged
merged 30 commits into from
Dec 10, 2024

Conversation

GernotMaier
Copy link
Member

@GernotMaier GernotMaier commented Aug 17, 2023

This pull requests simplifies installation and usage of V2DL3 significantly.

A simple pip install v2dl3 should do all and then we call the three scripts as command line tools:

  • v2dl3-vegas
  • v2dl3-generate-index-file
  • v2dl3-eventdisplay

It is very nice that we can use the same installation for eventdisplay and vegas flavours (at this point only for pip; could do this like also in future for the conda environments).

Replaces also setup.py by the more modern pyproject.toml approach.

Other changes:

  • updates README accordingly
  • add github action workflow for deployment to pypi (at this point to TestPyPi, see here to see how nice it will look like)

@GernotMaier GernotMaier self-assigned this Aug 17, 2023
@GernotMaier
Copy link
Member Author

@matthew-w-lundy @deividribeiro - could you have a look why the vegas workflow is failing? I am not sure I understand the different step of docker generation and pip installation. Maybe it is a small thing only - having this PR included would really by nice!

@matthew-w-lundy
Copy link
Contributor

Appears to be how in the docker image we are pip installing. Locally running the instructions work in a clean conda environment. I will see if I can modify the test to reasonably still pass with changes to the install.

@GernotMaier
Copy link
Member Author

@matthew-w-lundy - any progress on this issue?

Being able to merge this PR would really be nice, as it simplifies installation.

@matthew-w-lundy
Copy link
Contributor

Taking a look at this again today to try and resolve

@deividribeiro
Copy link
Contributor

@GernotMaier I've uploaded a new vegas docker image to the workflow - can you resolve the conflict in this PR to trigger the actions so that we may be able to merge? The new docker image is updated to a python3 environment so the toml/pip issues should go away.

@GernotMaier GernotMaier marked this pull request as ready for review November 14, 2024 09:42
@GernotMaier
Copy link
Member Author

@deividribeiro - the v2dl3-vegas-CI is failing. Can you have a look?

@deividribeiro
Copy link
Contributor

Thanks! This issue looks fixable. It is missing some custom EAs that were part of the original docker image from Tyler, so I have to hunt down those files and place back in. But at least it appears to have passed the original issues with pypi. @tmcahill if you have those files or can point me to them - let me know and I will update.

@tmcahill
Copy link
Contributor

Good to use image history if possible, I can probably get them if not. Will follow up with @deividribeiro on slack.

@deividribeiro
Copy link
Contributor

wooo! done. This PR should now be ok.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 15 changed files in this pull request and generated 1 suggestion.

Files not reviewed (7)
  • utils/v2dl3-eventdisplay-docker/Dockerfile: Language not supported
  • utils/v2dl3-vegas-docker/Dockerfile_vegas_ci: Language not supported
  • utils/v2dl3-vegas-docker/Dockerfile_vegas_v2dl3_arm64: Language not supported
  • utils/v2dl3-vegas-docker/Dockerfile_vegas_v2dl3_x64: Language not supported
  • utils/vegas_docker_test_runs.sh: Language not supported
  • setup.py: Evaluated as low risk
  • .github/workflows/pypi.yml: Evaluated as low risk

README.md Outdated
```

> [!NOTE]
> This This will replaced in near future by a pip install from PyPI.
Copy link
Preview

Copilot AI Dec 10, 2024

Choose a reason for hiding this comment

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

Duplicate 'This' should be removed.

Suggested change
> This This will replaced in near future by a pip install from PyPI.
> This will be replaced in near future by a pip install from PyPI.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@deividribeiro deividribeiro merged commit bd500ce into main Dec 10, 2024
4 checks passed
@deividribeiro deividribeiro deleted the pypi-eventdisplay branch December 10, 2024 16:47
@GernotMaier GernotMaier linked an issue Dec 10, 2024 that may be closed by this pull request
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.

Version numbering
4 participants