explosion / weasel

🦦 weasel: A small and easy workflow system
MIT License
63 stars 8 forks source link

Initial implementation #3

Closed koaning closed 1 year ago

koaning commented 1 year ago

This is a work-in-progress PR.

Commands TODO:

adrianeboyd commented 1 year ago

The CI is already configured for azure pipelines, so I don't see any reason to use github actions for this?

You'd need to add the dev requirements to requirements.txt (mypy and pytest, at least) and then move the tests into weasel/tests so the tests are installed with the package, which is our normal package setup.

The CI steps could be improved in a few minor ways (mainly cleaner tests of the package requirements), but the basics should all be working at this point.

koaning commented 1 year ago

Status report!

I've been able to run all commands locally except for the push and pull commands. I think I've copied the relevant utility methods over, but there are ample opportunities to clean up the repo. I just copied all the existing methods from spaCy, and I'm pretty sure we don't need everything the way it currently is.

There are a few things that we should discuss:

  1. Do we want confection to be a dependency? Some parts of the code used the Config object from Thinc, which I've replaced with confection for now.
  2. Are there extra features we might want to add to the project? We might want to add progress bars for the assets command.
  3. How do we want to go about cleaning this repo? A big cleanup should probably happen, but part of me would prefer if we maybe first write some unit tests before we make big changes.
  4. We can probably delete some of the old project's code in spaCy in the future, who will pick that up?
rmitsch commented 1 year ago
  1. I think that makes sense.
  2. I'd get it shipped as-is before adding new features.
  3. Agreed - first unit tests, then clean-up (clean-up can IMO happen in a later PR, unit tests should be included now).
  4. We don't have to decide that now, I think?
koaning commented 1 year ago

We don't have to decide that now, I think?

That's true. But it'd be helpful if the person who cleans up spaCy is also involved with the effort here. Which is mainly why I mentioned it.

polm commented 1 year ago

Remaining test failures are due to lack of Pathy support for 3.11, so I'll work on moving this to cloudpath like in https://github.com/explosion/spaCy/pull/11750.

polm commented 1 year ago

OK, at this point:

I think this is good to go ahead to merge as a base for further PRs like:

And so on.

adrianeboyd commented 1 year ago

I think I'd prefer _util and then non-underscore names for the individual modules?

polm commented 1 year ago

That makes sense, it means less underscores all round too.

svlandeg commented 1 year ago

As discussed internally, let's close this PR and then:

To everyone having worked on this PR, in particular @koaning: thanks for all the work on it and sorry that this got sidelined a bit and needs to be redone! I think we got a bit carried away trying to do too much in this PR but the history is all here so we can definitely build on that and use this prior work when going forward with the separate PRs 🙏

koaning commented 1 year ago

No worries from my end. I'm just happy that weasel is going to be a thing soon 🙂 !

svlandeg commented 1 year ago

cf https://github.com/explosion/weasel/pull/9