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 gooey support #130

Merged
merged 7 commits into from
Nov 26, 2024
Merged

add gooey support #130

merged 7 commits into from
Nov 26, 2024

Conversation

yucongalicechen
Copy link
Collaborator

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

@sbillinge ready for some feedback!

from diffpy.labpdfproc.functions import CVE_METHODS, apply_corr, compute_cve
from diffpy.labpdfproc.tools import known_sources, load_metadata, preprocessing_args
from diffpy.utils.parsers.loaddata import loadData
from diffpy.utils.scattering_objects.diffraction_objects import XQUANTITIES, Diffraction_object


def define_arguments():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we need to redefine the input and zscan file arguments for gui, I think it's easier if we put all arguments in a separate function, so that later when we update the arguments we only need to update this.

"folder ./data)."
),
"nargs": "+",
"widget": "MultiFileChooser",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we have cli we support a mixture of multiple files, folders, and wildcards. In gui I think we can only choose one of multiple file or folder for one argument. I can maybe add another argument for selecting folders?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, if that is the case in teh gui let's have them as separate inputs

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this looks really great! Congrats. If you make an issue to update the docs/tutorial I can merge this. Or else we should put the updates to the docs on this PR and merge it together.

"folder ./data)."
),
"nargs": "+",
"widget": "MultiFileChooser",
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, if that is the case in teh gui let's have them as separate inputs

src/diffpy/labpdfproc/labpdfprocapp.py Show resolved Hide resolved
@sbillinge
Copy link
Contributor

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
Copy link
Contributor

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

Also, a similar issue that remains open:

chriskiehl/Gooey#920

@sbillinge
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

bobleesj commented Nov 20, 2024

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
Copy link
Contributor

bobleesj commented Nov 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

I just checked that Python 3.13 works on my computer

Thanks. I will look more into this tmr!

@bobleesj
Copy link
Contributor

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

@yucongalicechen
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

@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)

@sbillinge sbillinge merged commit a2572da into diffpy:main Nov 26, 2024
3 checks passed
@yucongalicechen yucongalicechen deleted the gui branch December 7, 2024 20:12
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.

add gooey support so it can be run with a gui
3 participants