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

silence deprecation warnings #182

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

MarDiehl
Copy link
Contributor

they show up for:

  • re (python 3.9.7)
  • semver 2.13.0

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

We will probably need to bump the version (in spython/version.py) and add a note to the CHANGELOG (.md) in the root if it's not too much trouble!

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

Also can you please confirm the changes still work in older versions of Python (3.9.x is fairly new) and that semver here

INSTALL_REQUIRES = (("semver", {"min_version": "2.8.0"}),)
is set to the minimum that will be allowed with this UI change?

@MarDiehl
Copy link
Contributor Author

I think the most sustainable way to ensure a minimum python version is to parameterize the github actions to use various python versions. I would implement this first in a separate branch if that is ok

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

If you'd be willing to do that, it would be great.

@MarDiehl
Copy link
Contributor Author

I have a branch that runs the tests with python 3.7-3.9.
Unfortunately, I cannot install singularity on the debian containers and needed to mark some tests as xfail. For most of them, that was a clear task ('singularity not found') but for two I'm not sure:

https://github.com/MarDiehl/singularity-cli/runs/3758070266?check_suite_focus=true

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

ah I see the issue. We don't run the singularity python tests on GitHub CI - singularity doesn't install there with appropriate permissions to do so. You'll want to add the matrix logic to .circleci/config.yaml.

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

But I haven't tried it in years - if you want to give a shot in GitHub you could try https://github.com/marketplace/actions/setup-singularity.

@MarDiehl
Copy link
Contributor Author

It almost works: https://github.com/MarDiehl/singularity-cli/runs/3759771046?check_suite_focus=true, there are only two issues

  1. sudo singularity does not work. If sudo is required, xfail would make sense here. sudo does not exist on all systems anyways
  2. A test for a substring should be a test for a substring in a list of lists. This is easy to implement, but I don't understand the consequences for other setups

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

sudo should work in actions - that failure looks like you need the singularity install location in sudos path. That could be done here or maybe better alongside the action to install it.

@MarDiehl
Copy link
Contributor Author

I've included a workaround:
https://github.com/MarDiehl/singularity-cli/runs/3760571852?check_suite_focus=true

any comments on the remaining fails? I would assume that the extra information INFO: Convert SIF file to sandbox... and INFO: Cleaning up image... is related to some debug settings.

@vsoch
Copy link
Member

vsoch commented Sep 30, 2021

That should be easy to fix! You can add:

if isinstance(result['message'], list):
        result['message'] = "".join(result['message'])

to test_client.py for the test_execute_with_return_code function.

spotted with re (python 3.9.7) and semver 2.13.0.
VersionInfo.parse is available since semver 2.8.1, see
https://github.com/python-semver/python-semver/releases/tag/2.8.1
@vsoch vsoch force-pushed the remove-deprecation-warnings branch from 8113c9a to 5b61a5b Compare October 2, 2021 01:17
@vsoch vsoch merged commit bb278aa into singularityhub:master Oct 2, 2021
@vsoch
Copy link
Member

vsoch commented Oct 2, 2021

Thanks again! It looks much better without the warnings.

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.

2 participants