Skip to content

switch from docopt to docopt-ng#927

Merged
thequilo merged 3 commits intoIDSIA:masterfrom
n-gao:ngao/docopt-ng
Aug 26, 2024
Merged

switch from docopt to docopt-ng#927
thequilo merged 3 commits intoIDSIA:masterfrom
n-gao:ngao/docopt-ng

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