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

Fix test #1642

Merged
merged 14 commits into from
Feb 2, 2025
Merged

Fix test #1642

merged 14 commits into from
Feb 2, 2025

Conversation

sharkAndshark
Copy link
Collaborator

@sharkAndshark sharkAndshark commented Jan 4, 2025

Try to fix #1630

  • Back to postgis:14-3.3
  • Update pmt2_0_0_0.png.txt. We need more research here. It's weired.

@nyurik
Copy link
Member

nyurik commented Jan 8, 2025

I am seeing a very strange issue - the martin_cp result differs on the GitHub runners vs my local runs. This is specifically apparent in the cp_flat.mbtiles:

❯ diff failed/tests/output/martin-cp/flat_summary.txt expected/martin-cp/flat_summary.txt 
7,9c7,9
<     1 |         4 |      338B |      705B |      439B | -180,-85,180,85
<     2 |         5 |      123B |      690B |      356B | -90,-67,180,67
<     3 |         8 |       75B |      727B |      245B | -45,-41,180,67
---
>     1 |         2 |      150B |      172B |      161B | -180,-85,0,85
>     2 |         4 |      291B |      690B |      414B | -90,-67,90,67
>     3 |         7 |       75B |      727B |      263B | -45,-41,90,67
13c13
<   all |       127 |       75B |      727B |      197B | -180,-85,180,85
---
>   all |       123 |       75B |      727B |      190B | -180,-85,180,85

P.S. what the above means is that the failed (CI) runs generate fewer tiles than my local installation.

@sharkAndshark
Copy link
Collaborator Author

Yes I noticed and trying to figure out

@nyurik
Copy link
Member

nyurik commented Jan 8, 2025

how can it be that martin-cp sees fewer tiles in CI? Could it be a postgres issue (some magical SQL returns a different result?)... or something else? This is probably the biggest concern tbh - as it shows that in CI we have different content somehow

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 9, 2025

as_mvtgeom filter more than ever may be? Need to dig into

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 9, 2025

debugging

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 10, 2025

The failed test is running on PostgreSQL16.6.0 / PostGIS 3.5.1.
But you could easily duplicate it on PostgreSQL 17.2.0 / PostGIS 3.5.1 (no docker images whose tag is exactly equal to 16.6.0+3.5.1 so I go to 17-3.5 )

  db-latest:
    # This should match the version of postgres used in the CI workflow
    image: postgis/postgis:17-3.5
    restart: unless-stopped
    ports:
      - '${PGPORT:-5411}:5432'
    command:
      - '-c'
      - 'max_connections=200'
    environment:
      # POSTGRES_* variables are used by the postgis/postgres image
      # PG_* variables are used by psql
      - POSTGRES_DB=db
      - POSTGRES_USER=postgres
      - POSTGRES_PASSWORD=postgres
      - PGDATABASE=db
      - PGUSER=postgres
      - PGPASSWORD=postgres
    volumes:
      - ./tests/fixtures:/fixtures
      - ./tests/fixtures/initdb-dc.sh:/docker-entrypoint-initdb.d/20_martin.sh
# a command added in jsutfile in this PR
just test-latest

So It seems that the new version PG/PostGIS return more than ever.

@@ -450,6 +450,20 @@ mod tests {

use super::*;

#[test]
fn temp_test_tile_ranges() {
let bbox = Bounds::from_str("-2, -1, 142.84, 45").unwrap();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a temp test here(would be removed before merge). We could see the tile range is same with our local machine.

@nyurik
Copy link
Member

nyurik commented Jan 10, 2025

I think we need to use the same version of postgres in both CI and in our local development - for consistency. I also think we should (for now) stay at an older version, e.g. 15, just so that we don't analyze too many things at the same time - and rebless the tests and release. Once done, we should figure out why v16+ (?) generates significantly different results - as this might be a major bug in our ST_AsMVT tile generation

@sharkAndshark
Copy link
Collaborator Author

Totally agree.

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 11, 2025

I test each of them, and the result is interesting....:

pg postgis features in pg tile ordering changed mbtiles content changed
14 3.3.2 no no
15 3.3.4 yes no
16 3.3 no docker image no docker image
17 3.3 no docker image no docker image
14.13.0 3.4.3 no no
15.8 3.4.3 yes no
16 3.4 yes no
17 3.4 yes no
15 3.5 yes yes
16 3.5 yes yes
17 3.5 yes yes

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 12, 2025

PostGIS14-3.5_cp_flat.mbtiles and PostGIS14-3.3_cp_flat.mbtiles
PostGIS14-3.3_and_14-3.5_cp_flat.zip

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 12, 2025

z x y pg_version source st_asmvt is_empty
1 1 1 14-3.3 table_source (BLOB) 2 bytes yes
1 1 0 14-3.3 table_source (BLOB) 2 bytes yes
1 1 1 14-3.5 table_source (BLOB) 607 bytes no
1 1 0 14-3.5 table_source (BLOB) 1.18 KB no

The generated SQL in log are same:

SELECT
      ST_AsMVT(tile, 'table_source', 4096, 'geom')
    FROM (
      SELECT
        ST_AsMVTGeom(
            ST_Transform(ST_CurveToLine("geom"::geometry), 3857),
            ST_TileEnvelope($1::integer, $2::integer, $3::integer),
            4096, 64, true
        ) AS geom
        , "gid"
      FROM
        "public"."table_source"
      WHERE
        "geom" && ST_Transform(ST_TileEnvelope($1::integer, $2::integer, $3::integer, margin => 0.015625), 4326)
      
    ) AS tile;

@sharkAndshark
Copy link
Collaborator Author

Remember we have a debug.html.

With postgis:14-3.3

image

With postgis:14-3.5

image

@sharkAndshark
Copy link
Collaborator Author

sharkAndshark commented Jan 12, 2025

I see nothing wrong in our generated SQL.

@sharkAndshark sharkAndshark force-pushed the fix-ci branch 3 times, most recently from 0bb527d to 4d2d0aa Compare January 27, 2025 14:33
@sharkAndshark sharkAndshark force-pushed the fix-ci branch 3 times, most recently from e4d53a8 to 0c809e6 Compare January 27, 2025 16:01
@sharkAndshark sharkAndshark marked this pull request as ready for review January 27, 2025 16:29
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

FYI: we got one user on slack who complained about lines not showing up on high zoom levels => it turned out to be postgis fault and the issue is fixed in newer versions of the DB..

id: pg
with: { username: 'test', password: 'test', database: 'test' }
with: { username: 'postgres', password: 'postgres', database: 'test',postgres-version: 14, port: 34837, postgis_version: 3.3.3}
- name: Install and run Postgis (Ubuntu)
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be on the same page on this and not loose why this code exists to time:
Why did you split it here? 🤔

Copy link
Collaborator Author

@sharkAndshark sharkAndshark Feb 2, 2025

Choose a reason for hiding this comment

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

  • With services we could set up various version of PostGIS/Postgres very easily, but only Linux runner support it..
  • Setting PostGIS version is only supported for Windows runner by now with nyurik/[email protected]. I'm working on for macos

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sometimes I doubt maybe test only on Ubuntu is enough...

@sharkAndshark sharkAndshark merged commit 31b781c into maplibre:main Feb 2, 2025
36 checks passed
@sharkAndshark sharkAndshark deleted the fix-ci branch February 3, 2025 14:18
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.

Made diff about *.pbf ignore features order
3 participants