diffpy / diffpy.labpdfproc

apps for preprocessing laboratory x-ray data before sending to pdfgetx3
Other
1 stars 10 forks source link

add gooey support #130

Closed yucongalicechen closed 4 days ago

yucongalicechen commented 1 week ago

closes #57 It's working for both CLI arguments and GUI. Here're some screenshots. gui1 gui2 gui3

@sbillinge ready for some feedback!

sbillinge commented 1 week ago

we have to find some kind of workaround to pin testing to py 3.12 until the Gooey dependency is updated. This won't be so easy because of the centralized way we are doing testing at the moment, but let's try and find a way to just test this up to 3.12.

bobleesj commented 1 week ago

@sbillinge it appears that the latest conda-forge supports py310.. https://anaconda.org/conda-forge/gooey/files

Also, a similar issue that remains open:

https://github.com/chriskiehl/Gooey/issues/920

sbillinge commented 1 week ago

I am running gooey on Regolith in a 3.12 env. So it is not a problem with the code per se, but the issue is how we run our tests and design our requirements. ATM I think we are pulling a file from release-docs that is uniform across projects and we migrated at some point to it testing on 3.13, so downgrading labpdfproc will be a bit messy.....and we will have to think of how to re-update it later when gooey comes back up to speed (I volunteered on the project to do that but didn't hear back yet).

yucongalicechen commented 1 week ago

I am running gooey on Regolith in a 3.12 env. So it is not a problem with the code per se, but the issue is how we run our tests and design our requirements. ATM I think we are pulling a file from release-docs that is uniform across projects and we migrated at some point to it testing on 3.13, so downgrading labpdfproc will be a bit messy.....and we will have to think of how to re-update it later when gooey comes back up to speed (I volunteered on the project to do that but didn't hear back yet).

I read online that we can use matrix.python-version to test different Python versions. I can try that. Basically it can test everything for one Python version, and everything else except for the incompatible package for another version. I put everything we discussed here in issues #131, #132, #133.

bobleesj commented 1 week ago

I am running gooey on Regolith in a 3.12 env. So it is not a problem with the code per se, but the issue is how we run our tests and design our requirements. ATM I think we are pulling a file from release-docs that is uniform across projects and we migrated at some point to it testing on 3.13, so downgrading labpdfproc will be a bit messy.....and we will have to think of how to re-update it later when gooey comes back up to speed (I volunteered on the project to do that but didn't hear back yet).

I read online that we can use matrix.python-version to test different Python versions. I can try that. Basically it can test everything for one Python version, and everything else except for the incompatible package for another version. I put everything we discussed here in issues #131, #132, #133.

@yucongalicechen we have matrix testing configured https://github.com/diffpy/diffpy.labpdfproc/blob/main/.github/workflows/matrix-and-codecov-on-merge-to-main.yml in this repository. But, again, it's testing against 3.11, 3.12, 3.13.

@sbillinge One possibility is that we could dynamically override default Python versions (3.11, 3.12, 3.13) here and just make sure we remove it later:

From

jobs:
  matrix-coverage:
    uses: Billingegroup/release-scripts/.github/workflows/_matrix-and-codecov-on-merge-to-main.yml@v0
    with:
      project: diffpy.labpdfproc
      c_extension: false
      headless: false

to

jobs:
  matrix-coverage:
    uses: Billingegroup/release-scripts/.github/workflows/_matrix-and-codecov-on-merge-to-main.yml@v0
    with:
      project: diffpy.labpdfproc
      c_extension: false
      headless: false
      python_versions: 3.10, 3.11 # Override
bobleesj commented 1 week ago

but the issue is how we run our tests and design our requirements

Another possibility is that instead of conda install in our CI, do we pip install, it will probably work for Py3.13 since @yucongalicechen created this PR using pip install of gooey.

sbillinge commented 1 week ago

Thankd @bobleesj I think either of those solutions can work. The basic idea is that we for sure will have to make some local change in the GitHub workflow and then make an issue to change it back later, but what is the minimal change that just kind of gets the job done. But the update needs to be in the local directory, not in release-packages (I forget the name exactly) it it compromises tests across all our stack.

bobleesj commented 1 week ago

@yucongalicechen

Building wheels for collected packages: wxpython
  Building wheel for wxpython (pyproject.toml): started
  Building wheel for wxpython (pyproject.toml): finished with status 'error'
  error: subprocess-exited-with-error

seems like wxpython pip install fails here in the CI. Did you test pip install using conda env with Python 3.13 before?

sbillinge commented 1 week ago

@yucongalicechen

Building wheels for collected packages: wxpython
  Building wheel for wxpython (pyproject.toml): started
  Building wheel for wxpython (pyproject.toml): finished with status 'error'
  error: subprocess-exited-with-error

seems like wxpython pip install fails here in the CI. Did you test pip install using conda env with Python 3.13 before?

It gets complicated but we could conda install wx then pip install gooey, or just only test up to 3.12

yucongalicechen commented 1 week ago

@yucongalicechen

Building wheels for collected packages: wxpython
  Building wheel for wxpython (pyproject.toml): started
  Building wheel for wxpython (pyproject.toml): finished with status 'error'
  error: subprocess-exited-with-error

seems like wxpython pip install fails here in the CI. Did you test pip install using conda env with Python 3.13 before?

I just checked that Python 3.13 works on my computer

bobleesj commented 1 week ago

I just checked that Python 3.13 works on my computer

Thanks. I will look more into this tmr!

bobleesj commented 5 days ago

@yucongalicechen Let's try re-running it now. https://github.com/diffpy/diffpy.labpdfproc/pull/136 is merged so that it the linux CI can build wxpython.

yucongalicechen commented 5 days ago

@yucongalicechen Let's try re-running it now. #136 is merged so that it the linux CI can build wxpython.

Working nicely!! Thanks Bob!!

yucongalicechen commented 5 days ago

@sbillinge I think this PR is ready to be merged. I've added issues #131 and #132 to take care of the docs and input argument.

bobleesj commented 5 days ago

@yucongalicechen it just takes a bit of time (28 mins) to build wxpython here for Linux.. (uses sdist instead of whl files since PyPI provides no linux whl file)