Fixes for Modular Converter on Windows #34266
Conversation
|
|
hlky
force-pushed
the
modular-converter-windows
branch
from
2496fda to
6b8b5ae
Compare
utils/modular_model_converter.py
Outdated
| relative_path = re.search( | ||
| rf"(src{os.sep}transformers{os.sep}.*|examples{os.sep}.*)", os.path.abspath(modular_file) | ||
| rf"(src{sep}transformers{sep}.*|examples{sep}.*)", os.path.abspath(modular_file) |
There was a problem hiding this comment.
I think we'd better use os.path.join no? This would directly support correct separation?
There was a problem hiding this comment.
It's the same separation as os.sep with os.path.join. This is a python/regex issue due to the combination of raw and formatted.
An alternate solution would be replacing \\ on os.path.abspath, then regex can just use /:
relative_path = re.search(
r"(src/transformers/.*|examples/.*)", os.path.abspath(modular_file).replace("\\", "/")
).group(1)There was a problem hiding this comment.
looks simpler let's go with this!
hlky
force-pushed
the
modular-converter-windows
branch
from
6b8b5ae to
c47af79
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Thanks for your contribution! |
* Separator in regex * Standardize separator for relative path in auto generated message * open() encoding * Replace `\` on `os.path.abspath` --------- Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
What does this PR do?
This PR fixes some issues with Modular Converter on Windows.
rf"(src{os.sep}transformers{os.sep}.*|examples{os.sep}.*)"results in(src\transformers\.*|examples\.*), due to\tand\.this regex fails, we replace\onos.path.abspathinstead, and use(src/transformers/.*|examples/.*)as the regex.2. Relative path in auto generated message. On Windows this generates asThis file was automatically generated from src\transformers\models\florence2\modular_florence2.py.This could potentially cause issues elsewhere, and it's better to be standardized, so whenos.sep==\we replaceos.sepwith/.openencoding. On Windows the default encoding iscp1252which doesn't work with🚨, so we useencoding=utf-8instead for allopen. This particular fix is in effect in many other places in the codebase.Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@ArthurZucker