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

[ENH] Setup: translated packages #307

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Sep 2, 2024

@markotoplak, could you check that this is what we want?

It should work like in Image Analytics, except that in Image Analytics we assume that Trubar is installed (because Orange requires it), while here we don't.

This should work as in Image Analytics, except that it does not require Trubar.

If you confirm this, I'll make the equivalent PR for orange-widget-base and orange3.

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.60%. Comparing base (d3d2588) to head (1d92da3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   76.61%   76.60%   -0.01%     
==========================================
  Files         100      100              
  Lines       21207    21207              
==========================================
- Hits        16247    16246       -1     
- Misses       4960     4961       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@markotoplak
Copy link
Member

Firstly, developers don't need to worry about this, because this does not in any way influence pip install -e ..

If we do not require trubar as setup_requires (or in pyproject.toml, see biolab/orange3-imageanalytics/pull/247) then:

  • Anyone installing from a source distribution (.tar.gz) will get non-translated Orange. Due to the default build isolation, there will be no trubar available even if users have it.
  • Anyone installing a trubarized wheel (.whl) will get the translated version.
  • Building a trubarized wheel is possible with python setup.py bdist_wheel (if, of course, trubar is installed) but not with python -m build --wheel

Because this is a pure Python package, pip will (I presume) always default to .whl, which, if built correctly (which we can ensure), will be translated. But Orange3 is not pure Python; we may have users on CPU architectures or operating systems where we do not build wheels, so there even .tar.gz should be working properly.

I suggest adding Trubar as a packaging dependency so that setup.py is consistent. (This would require making Trubar Python 3.8 compatible.)

An alternative could be to make a patch file for setup.py that we would apply before building packages. Then, no one building these packages for themselves would have to do anything with Trubar. I do not think Trubar is problematic anyway because typical dev installs are editable.

What do you think about this, @ales-erjavec?

@markotoplak
Copy link
Member

Or we could even raise required python to 3.9 or even 3.10 (per https://numpy.org/neps/nep-0029-deprecation_policy.html we are in 3.10+ times).

@janezd
Copy link
Contributor Author

janezd commented Sep 5, 2024

This would require making Trubar Python 3.8 compatible.

Trubar cannot go below 3.9 because libcst requires it (https://pypi.org/project/libcst/). And it has to use a recent libcst to support the latest Pyhon syntax features.

Due to the default build isolation, there will be no trubar available even if users have it.

One can avoid build isolation by using --no-build-isolation.

I am bit lost. Let us discuss tomorrow what exactly do we want. (We probably already did, but I forgot. Also, we may be smarter than two months ago.)

@markotoplak
Copy link
Member

If you are fine with libcst>=1.1 (from last year), then it is also python 3.8 compatible.

@janezd
Copy link
Contributor Author

janezd commented Sep 5, 2024

If you are fine with libcst>=1.1

I think I'm not. I don't remember the exact reason, but I know that I was forced to use a newer libcst and drop support for 3.8.

  • Maybe it had something to do with wheels. Libcst is partially written in Rust, so you need to use binary wheels (or install Rust compiler). Even if we could solve this for now, using newer OSs will eventually force us to upgrade.
  • It may had to do with wheels for CI. Perhaps I would have to configure it to use an earlier version of Linux.
  • I vaguely remember that I needed a feature that is only available in 1.2 (or in 1.1?!), or hasn't worked correctly in earlier versions. It may had to do with the way trubar puts its own imports after imports from future.

We can try. Let us discuss this tomorrow; perhaps this is moot anyway.

@markotoplak markotoplak changed the title Setup: Add translation if trubar is available Setup: add translations Sep 20, 2024
@markotoplak markotoplak changed the title Setup: add translations [ENH] Setup: translated packages Sep 20, 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.

3 participants