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

Add progress to requirements for testing #166

Merged
merged 2 commits into from
Sep 11, 2024
Merged

Conversation

nsryan2
Copy link
Member

@nsryan2 nsryan2 commented Jul 10, 2024

As mentioned in #128, the install of PyNE was failing due to a missing package called progress. With the error:

# PyNE Environment Settings

  export PATH="/home/circleci/.local/bin:${PATH}"
Traceback (most recent call last):
  File "/home/circleci/.local/bin/nuc_data_make", line 4, in <module>
    from pyne.dbgen.nuc_data_make import main
  File "/home/circleci/.local/lib/python3.10/site-packages/pyne/dbgen/nuc_data_make.py", line 4, in <module>
    from pyne.utils import QA_warn
  File "/home/circleci/.local/lib/python3.10/site-packages/pyne/utils.py", line 9, in <module>
    from progress.bar import Bar
ModuleNotFoundError: No module named 'progress'

Exited with code exit status 1

This PR adds progress to the requirements.txt file so the CI will install it for testing. This PR also removes some white space from the config.yml file.

@nsryan2
Copy link
Member Author

nsryan2 commented Aug 30, 2024

Converting to a draft because adding progress led to needing Cymetric, which requires Cyclus, and I'm not sure how this integration was intended because that doesn't seem to have been part of it in the past.

@katyhuff do you think that I would be going down a rabbit hole here by building Cyclus for the tests (it seems necessary)? I'm not sure I understand how the tests were passing before.

@katyhuff
Copy link
Member

Sorry -- I just realized I was mentioned in this issue. For the future, pleas actually assign me to review if you need my review on something. I turned off notifications for mentions a long time ago when it got to be too many repos/orgs.

Thanks for raising this in our meeting today @nsryan2 . I think it's worth making the tests pass, however it needs to happen.

@katyhuff
Copy link
Member

However, since the tests were failing before this PR, and since this PR advances the state of things, I'm happy to support merging this PR for now, rather than making the perfect the enemy of the good. Is that desirable @nsryan2 ?

@katyhuff katyhuff marked this pull request as ready for review September 10, 2024 22:51
@nsryan2
Copy link
Member Author

nsryan2 commented Sep 11, 2024

@katyhuff Absolutely, after our conversation yesterday I started working on an update. I'll request reviews in the future! I think these changes will be a part of the solution I come up with, so I'll merge them here.

@nsryan2 nsryan2 merged commit 9c0f037 into arfc:master Sep 11, 2024
@nsryan2 nsryan2 deleted the cci_progress branch September 11, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants