Skip to content

fix: load paths, imports, transpile, and CI permissions - #2128

Open
Chessing234 wants to merge 5 commits into
MIT-LCP:mainfrom
Chessing234:fix/load-path-infra
Open

fix: load paths, imports, transpile, and CI permissions#2128
Chessing234 wants to merge 5 commits into
MIT-LCP:mainfrom
Chessing234:fix/load-path-infra

Conversation

@Chessing234

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • pytest tests/test_transpile.py

Made with Cursor

@Chessing234

Copy link
Copy Markdown
Contributor Author

Infra-only follow-up to the too-large #2124. Concept changes stay in #2126 / separate PRs.

@alistairewj alistairewj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the changes are scattered and not very cohesive but with some cleaning up we can merge

Comment thread .github/workflows/lint_sqlfluff.yml Outdated
Comment thread mimic-iii/buildmimic/duckdb/import_duckdb.sh Outdated
Comment thread src/mimic_utils/sqlglot_dialects/duckdb.py Outdated
Comment thread mimic-iii/buildmimic/postgres/Makefile Outdated
Comment thread mimic-iv-cxr/txt/chexpert/run_chexpert_on_files.sh Outdated
Comment thread .github/workflows/lint_sqlfluff.yml Outdated
Comment thread mimic-iv-cxr/txt/negbio/run_negbio.sh
Comment thread mimic-iii/buildmimic/sqlite/import.py Outdated
Comment thread mimic-iv/concepts/validate_concepts.sh Outdated
Quote/escape datadirs and passwords in loaders, accept plain CSV in duckdb/sqlite imports, utf-8 transpile writes, and don't fail sqlfluff annotate on fork PRs.
@Chessing234
Chessing234 force-pushed the fix/load-path-infra branch from a093a10 to 0d36c15 Compare July 29, 2026 15:28
@Chessing234

Copy link
Copy Markdown
Contributor Author

Addressed review feedback in 270b6d3:

  • SQLFluff: removed continue-on-error; added permissions: contents: read + checks: write; use ignore-unauthorized-error: true
  • Reverted duckdb usage text (no .tar.gz), DuckDB GENERATE_ARRAY transform + related concept/test, Makefile concepts target, CheXpert/NegBio defensive rewrites, and validate_concepts awk change
  • Removed sqlite TABLE_DTYPES from this PR (will split to a follow-up with CI as requested); kept plain .csv loading

@Chessing234

Copy link
Copy Markdown
Contributor Author

split the review asks into focused follow-ups: sqlfluff annotate permissions are in #2148; the sqlite import.py csv/.csv.gz discovery (with unit tests + ci) is in #2149. happy to trim the remaining infra bits on this branch further if you want them dropped.

@Chessing234
Chessing234 force-pushed the fix/load-path-infra branch from 427d504 to 3015382 Compare August 5, 2026 10:34
@acnimma

acnimma commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@alistairewj Ready for another look.

Your July 25 review asks are addressed as follows:

Ask Status
SQLFluff continue-on-error too blunt Removed; permissions + ignore-unauthorized-error moved to #2148
DuckDB .tar.gz usage text Reverted
DuckDB GENERATE_ARRAY transform Reverted
Makefile concepts target Reverted
CheXpert / NegBio defensive path edits Reverted
sqlite CSV discovery Split to #2149 with tests/CI
validate_concepts.sh awk / comment Reverted

This PR now keeps only the remaining load-path / transpile / duckdb import / validate_demo infra. Happy to cut anything else that still feels out of scope.

@Chessing234

Copy link
Copy Markdown
Contributor Author

reverted the make-concepts.sh quote-only hunks too. should be down to duckdb path escaping, postgres datadir/user quoting, sofa/oasis build order, transpile utf-8, and validate_demo tables. sqlfluff + sqlite csv bits stay in #2148 / #2149.

@Chessing234

Copy link
Copy Markdown
Contributor Author

@alistairewj resolved the review threads — ready for another look when you have time.

@Chessing234

Copy link
Copy Markdown
Contributor Author

@alistairewj re-review please when you get a chance — pushed after your review to address each point:

kept this PR scoped to load-path / transpile / import infra only.

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