Initial version of osrparse backed by osucore (rust parser) #37
Conversation
|
after getting this merged, will change the CI to publish on release, probably makes sense to enable it for python as well |
by osucore (written in rust), approximately 14x faster in serial parsing, and N x that in multicore/batch mode.
| if mode is GameMode.STD: | ||
| if mode == GameMode.STD: | ||
| keys = Key(keys) | ||
| event = ReplayEventOsu(time_delta, float(x), float(y), keys) | ||
| if mode is GameMode.TAIKO: | ||
| if mode == GameMode.TAIKO: | ||
| event = ReplayEventTaiko(time_delta, int(x), KeyTaiko(keys)) | ||
| if mode is GameMode.CTB: | ||
| if mode == GameMode.CTB: | ||
| event = ReplayEventCatch(time_delta, float(x), int(keys) == 1) | ||
| if mode is GameMode.MANIA: | ||
| if mode == GameMode.MANIA: | ||
| event = ReplayEventMania(time_delta, KeyMania(int(x))) | ||
|
|
There was a problem hiding this comment.
Does the rust parser break is checks? is is the recommended way to compare enums in python, so this would have to be a breaking change if we're unable to replicate it in rust. Would much prefer (almost to the point of it being a requirement) if is checks worked as-is.
There was a problem hiding this comment.
There are some ways I could get this to work, is checks will work if you directly compare IntFlags (ie. Mod.NoMod is Mod.NoMod) but will not work if one quantity is derived from an operation like (Mod.NoMod | Mod.Hidden) is (Mod.NoMod | Mod.Hidden), I might be able to figure out a way though it probably would get uglier, I'd have to precompute a whole bunch of mod combinations and make them const fields in rust, this would probably require some macros.
| @@ -53,10 +66,10 @@ def test_is_perfect_combo(self): | |||
|
|
|||
| def test_nomod(self): | |||
| for replay in self._replays: | |||
| self.assertEqual(replay.mods, Mod.NoMod, "Mod combination is wrong") | |||
| self.assertEqual(replay.mods.value, Mod.NoMod.value, "Mod combination is wrong") | |||
There was a problem hiding this comment.
I assume this means the rust version doesn't support comparing intflag enums? That's pretty bad as well. I wouldn't want to use .value every time I did a mod comparison.
There was a problem hiding this comment.
It does, I just forgot to change this test back
| @@ -350,7 +350,7 @@ class Replay: | |||
| rng_seed: Optional[int] | |||
|
|
|||
| @staticmethod | |||
| def from_path(path): | |||
| def from_path(path, use_legacy_parser=False, header_only=False): | |||
There was a problem hiding this comment.
| def from_path(path, use_legacy_parser=False, header_only=False): | |
| def from_path(path, *, use_legacy_parser=False, header_only=False): |
similarly for other static parsing methods.
Also unsure about exposing the legacy parsing at all. I think I'd prefer if we were entirely backed by osucore so we don 't have to maintain two versions.
There was a problem hiding this comment.
Sure, we could remove it, I just left it as an option in case there are parsing differences between the two methods, though the same backing objects are used between legacy and new now.
| install_requires=[ | ||
| "osucore>=0.1.11", | ||
| ], |
There was a problem hiding this comment.
this is pulling from pypi right now, right? Having a separate package for the rust parser seems like bad style. Is this the intended path forward for the actual release? How would releasing a new osrparse version go? Would we have to build and release a new osucore version first, then bump the osucore requirement in osrparse and build and release a new osrparse version?
Ideally this would all be self contained in one pypi package and require only one build/release to release a new version.
There was a problem hiding this comment.
Yeah, I originally did this so that development could eventually be decoupled, but this might make sense if oscucore is really every used through osrparse
|
Blocked by PyO3/pyo3#2384 |
approximately 14x faster in serial parsing, and N x that in multicore/batch mode.