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

Python 3.9 #3952

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Python 3.9 #3952

merged 5 commits into from
Nov 26, 2024

Conversation

stchris
Copy link
Contributor

@stchris stchris commented Oct 24, 2024

This includes the following changes:

  • changes the base image from ubuntu:20.04 to python:3.9, which is Debian based (Python 3.9 release notes)
  • runs tests in dev mode and enables tracemalloc to allow us to see deperecation warnings and resource misusage. This slows down test runs a little bit (maybe 10s on top of the 150s on my machine), but allows us to catch some warnings much earlier.
  • updates a deprecated way to get the current version

Note

Up for discussion: should we have a feature flag which allows us to turn on dev mode for the application? This way we could get deprecation and resource usage warnings from the real application in the staging env.

Copy link
Contributor

@tillprochaska tillprochaska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing related to transliteration that would need to be fixed, otherwise this looks good to me.

Dockerfile Outdated
Comment on lines 6 to 12
&& apt-get -qq --no-install-recommends -y install locales \
ca-certificates postgresql-client libpq-dev curl jq git \
libxml2-dev libxslt1-dev python3-dev \
&& apt-get -qq -y autoremove \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* \
&& localedef -i en_US -c -f UTF-8 -A /usr/share/locale/locale.alias en_US.UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is in scope for this PR, so feel free to ignore this: I ran apt list --installed in a stock python:3.9 container which included the following:

  • ca-certificates
  • libpq-dev
  • curl
  • git
  • libxml2-dev
  • libxslt1-dev

I think it would be nice to remove stuff from the list that isn’t strictly required, just to make maintenance easier and prevent confusion around why certain packages are installed in the future.

I removed the packages listed above from the Dockerfile and built the image without problems, so maybe we can just drop them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't bother to check! Will remove those!

Dockerfile Outdated
Comment on lines 6 to 12
&& apt-get -qq --no-install-recommends -y install locales \
ca-certificates postgresql-client libpq-dev curl jq git \
libxml2-dev libxslt1-dev python3-dev \
&& apt-get -qq -y autoremove \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* \
&& localedef -i en_US -c -f UTF-8 -A /usr/share/locale/locale.alias en_US.UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed that you removed python3-icu. While this is an optional dependency and Aleph will run without it, it is recommended for improved transliteration in some alphabets. This affects transliterations displayed in the UI, but also xref etc.

Didn’t want to commit directly to your PR, but here’s a test case that you can add to test_entities_api.py that succeeds if PyICU is installed and fail otherwise.

    def test_view_transliterate(self):
        role, headers = self.login(is_admin=True)

        data = {
            "id": "1",
            "schema": "Person",
            "properties": {
                "name": ["İlham Əliyev"],
            },
        }
        entity = self.create_entity(data, self.col)
        index_entity(entity)

        res = self.client.get(f"/api/2/entities/{entity.id}", headers=headers)
        assert res.json["properties"]["name"][0] == "İlham Əliyev"
        assert res.json["latinized"]["İlham Əliyev"] == "Ilham Aliyev"

You can install PyICU as a Debian package python3-icu or via pip as pyicu (but I think that will build it from source).

stchris and others added 2 commits November 26, 2024 11:50
Co-authored-by: Till Prochaska <1512805+tillprochaska@users.noreply.github.com>
@stchris stchris merged commit 43669e3 into develop Nov 26, 2024
3 checks passed
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