-
Notifications
You must be signed in to change notification settings - Fork 868
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
Remove gulp
from package data, code base and CI tests
#3789
Conversation
WalkthroughThe changes involve streamlining configurations, updating URLs, and enhancing error messages across the project files. These adjustments aim to improve setup clarity, documentation accuracy, and user guidance within the project components. Changes
|
@coderabbitai ignore |
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.
Should we even include them inside the code base?
i think it would be much better to remove and point people at the download page for GULP in a message to the user if pymatgen can't find the binary. but needs to be confirmed with @shyuep
Yes I agree with this. But the original link does not work on my side for some reason? pymatgen/pymatgen/command_line/gulp_caller.py Lines 1 to 4 in ff94faa
But this website from Curtin University seems to work (I'm not sure if these two point to the same package though): https://gulp.curtin.edu.au/index.html |
@@ -61,11 +60,6 @@ jobs: | |||
- name: Install uv | |||
run: micromamba run -n pmg pip install uv | |||
|
|||
- name: Copy GULP to bin |
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 removed tests for gulp
from CI because it's not being run (nor supported to run) anyway:
pymatgen/tests/command_line/test_gulp_caller.py
Lines 33 to 36 in e04444e
gulp_present = which("gulp") and os.getenv("GULP_LIB") and ("win" not in sys.platform) | |
# disable gulp tests for now. Right now, it is compiled against libgfortran3, which is no longer supported in the new | |
# Ubuntu 20.04. | |
gulp_present = False |
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
pymatgen/command_line/gulp_caller.py (1)
659-659
: The updated error message in theGulpCaller
constructor now helpfully includes a download link, which is a good user experience improvement.Consider adding more specific error handling for different failure scenarios, such as permissions issues or network problems, to provide more targeted guidance to the user.
…m vendored files in .gitattributes and from exclude patterns in .pre-commit-config.yaml
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.
thanks so much @DanielYang59. this is excellent!! will result in shorter install times from the next pymatgen
release onwards thanks to no longer downloading outdated gulp
files.
btw, there's this nice tool that allows you to get a list of files in a pypi package
Thanks for reviewing. Not sure about the installation time, but the package size is reduced by half :) The
Thanks for sharing, it would be very helpful.
I'm more concerned about autonomous package downloading in CI tests, but we would see when we need it because it's not supported by Ubuntu 20.04 afterwards anyway. |
gulp
from package datagulp
from package data, code base and CI tests
Exclude
gulp
from package datagulp
(11 yrs old) should not be included in the package data. And it's taking up half of the package size:After removal, the package size decreased from 6.9 MB to 3.0 MB.
Should we even include them inside the code base?
Two test files are included but I'm not seeing them inside
setup.py
?: