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

switch from docopt to docopt-ng #927

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

n-gao
Copy link
Contributor

@n-gao n-gao commented Aug 22, 2024

Since docopt is deprecated and hasn't been updated in 10 years, its regex is throwing errors in Python >3.12. This PR changes the dependency from docopt to docopt-ng, which is a maintained fork of docopt.

I also changed some unittests to work again. However, the unittests do not properly support numpy 2.0. To get them to run successfully, one has to install numpy<2.0.

In general, sacred could use some cleaning as it heavily relies on deprecated packages like pkgutil and pkg_resources. Another example is datetime.datetime.utcnow() which is now datetime.datetime.now(datetime.UTC).

@thequilo
Copy link
Collaborator

Hi @n-gao! I wasn't aware of this, it's a good idea to switch to a maintained version of docopt. And you also fixed the AWS mock.

I'm just wondering why the tests didn't get triggered. I don't like to merge this without making sure that it doesn't break the tests.

@thequilo
Copy link
Collaborator

In general, sacred could use some cleaning as it heavily relies on deprecated packages like pkgutil and pkg_resources. Another example is datetime.datetime.utcnow() which is now datetime.datetime.now(datetime.UTC).

I agree. There is a whole lot of old baggage that sacred carries around. There's more than just deprecated packages, but fixing all that would be a lot of work. I currently don't have much time to work on these things, so I would be grateful if you would provide PRs for this.

To get them to run successfully, one has to install numpy<2.0.

I'm also aware of this, and I think I know what has to be changed for it to work with numpy 2.0. I can prepare a PR for this.

@n-gao
Copy link
Contributor Author

n-gao commented Aug 23, 2024

So, the CI is not triggering here because it would run on your budget and with your secrets, you would have to first include pull_request as trigger to the CI. However, I enabled the tests on my fork:
https://github.com/n-gao/sacred/actions/runs/10521691384

The tests that are failing are due to numpy 2.0, with nump<2 they work.

I'm also aware of this, and I think I know what has to be changed for it to work with numpy 2.0. I can prepare a PR for this.

That would be amazing! This seems to be an actual breaking change and not just a deprecation warning.

@thequilo
Copy link
Collaborator

So, the CI is not triggering here because it would run on your budget and with your secrets, you would have to first include pull_request as trigger to the CI.

Hmm, then I understood the triggers wrong. I'll add that as well.

This seems to be an actual breaking change and not just a deprecation warning.

Yes, numpy2 has a few breaking changes. It also broke a lot of our internal code base. But the impact on sacred is not super large.

@n-gao
Copy link
Contributor Author

n-gao commented Aug 26, 2024

Tests are now passing! :)

@n-gao
Copy link
Contributor Author

n-gao commented Aug 26, 2024

Can we merge this and make a new release?

@thequilo
Copy link
Collaborator

Yes, looks good to me!

@thequilo thequilo merged commit 02b4e3d into IDSIA:master Aug 26, 2024
26 checks passed
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