feat: add collections validation and GFQL support

[lmeyerov]

Summary

• Add collections validator with strict/autofix behavior and GFQL wire normalization
• Expose collections API with validate/warn and plot-time URL param validation
• Add collections tests and clarify plan template location
• Move collections helpers to graphistry/collections.py and introduce typed models in graphistry/models/collections.py

Key Features

• g.collections(...) API for defining subsets via GFQL expressions with priority-based visual encodings
• Helper constructors graphistry.collection_set(...) and graphistry.collection_intersection(...)
• Support for showCollections, collectionsGlobalNodeColor, and collectionsGlobalEdgeColor URL params
• Automatic JSON encoding and GFQL AST/Chain/wire-protocol normalization

Validation Behavior

• Strict mode: Raises ValueError on invalid input
• Autofix mode (default): Drops invalid collections with warnings
• Set collections require an id field (server requirement) - missing IDs are warned/dropped, not auto-generated
• Intersection collections cross-validate that referenced set IDs exist
• GFQL parsing uses _wrap_gfql_expr as canonical implementation with precise exception handling

Testing

python -m pytest graphistry/tests/test_collections.py graphistry/tests/test_dataset_id_invalidation.py
./bin/lint.sh
./bin/mypy.sh

Review Comments Addressed

See comment for detailed breakdown of all review comments and their resolutions.

[mj3cheun]

in the interest of simplifying, would it be difficult to detect pre-encoded strings?

[lmeyerov]

yeah this surprised me: the reason it's like this is bc the base REST api takes AABBCC instead of ~css colors like we started to do elsewhere i believe (js color(xyz) normalizes to rgba('...'), handling '#AABBCC', 'silver', etc)

so maybe fix is to ship better colors in REST, and then this upgrades to that?

[mj3cheun]

i think thats a great idea to ship better colours in REST will add that to the list

i was talking about the encode: bool = True, though, which is If True, JSON-minify and URL-encode collections. Use False for pre-encoded strings.

i was thinking we could just detect pre-encoded strings (which i assume are URL encoded and can be detected by attempting a parse). then we can remove this parameter

[mj3cheun]

nitpick: would just get rid of the _coerce_collection_list function and put the contents in here, would get rid of checking is list is dict etc twice and make easier to read

[mj3cheun]

if collection_type was not already a string, im not sure that just typecasting it is going to produce a valid type string given this string must be either set or intersection

[mj3cheun]

actually nevermind, just saw the if collection_type not in ('set', 'intersection'): below, im thinking maybe combine this check with the check below, if collection_type is not string its all but guaranteed to fail and hit continue in the section below

[mj3cheun]

general comment, validate_mode = 'autofix' seems more likely to produce invalid output and/or result in a collection getting skipped than otherwise. to me its biggest value is typecasting stuff like id=1 to id="1" etc. not sure how much value there is in making it explicit, maybe we just have warn true/false and otherwise handle things silently?

[lmeyerov]

hmm, if they do something like g.plot(validate_mode='autofix'), what do we want to happen , or not happen, w/ collections?

that was mostly added b/c people keep having dirty data that fails arrow conversion and rather seeing it load vs fixing their data/cfg

[mj3cheun]

i feel this could be simplified by just feeding the GFQL into graphistry.compute.chain and seeing if it parses properly rather than setting up stuff here to determine if GFQL is valid or not

[mj3cheun]

@lmeyerov regarding general comment above: im thinking maybe instead of validate we call it strict with "true" or "false" where strict true will throw errors and false will pass over non-compliant collections

autofix sorta implies to me that we are "correcting" the data when invalid but we arent really i think? more just passing it over if there are any mistakes

if we feel we need to type cast we might want to just do it anyway strict true or not

[lmeyerov]

the issue with strict true/false is that's closer to what we had before, and users were complaining that they just wanted it to 'work', hence autofix (coerce++). validate=true/false is closer to what you're thinking, while 'autofix' (coerce) is "it'll run, but may not be what you want, but you said wanted soemthing that runs"

We therefore have leeway in what autofix does --- we just need to warn (if warn=auto/true), and do what. So the q is... what should it do wrt diff collections errors, if strong opinions about any params in any direction?

My default intuition is probably:

• drop collections with invalid gfql
• colors: random-but-deterministic? or neutral grey / transparent? (default black looks buggy)
• others: ??? disable / see if there's default values ?

[mj3cheun]

agree with that, i think we do the 3 bullets listed if strict (or whatever we want to call it) is false and its almost what we are doing right now. theres only 1 point i would make about the current implementation

• instead of just trying to typecast stuff (which a lot of the time doesnt work), i would prefer we do default values or in the case of colours, random colours

in summary its only the approach i think might need to change, not the intention

EDIT: if we do the above it actually gets closer to a true autofix than what it was before, so i dont have an issue with the name anymore

[aucahuasi]

Thanks for this PR, I left some comments and I think we need to solve this feedback before merge:
https://github.com/graphistry/pygraphistry/pull/874/changes#r2778233409

[lmeyerov]

Review Comments Addressed (commits 23d1f00, d2bdf02, 47d87e8)

HIGH Priority - Fixed

Comment	Author	Issue	Resolution
#2778239385	@aucahuasi	Dead code after return (line 232)	✅ Removed unreachable code
#2778238588	@aucahuasi	Validate len(sets_list) > 0	✅ Added empty sets validation in _normalize_sets_list
#2778244203	@aucahuasi	Cross-validate intersection set IDs	✅ Added _validate_intersection_references() post-loop validation
#2778233409	@aucahuasi	Consolidate GFQL parsing with collections.py	✅ _normalize_gfql_ops now calls _wrap_gfql_expr from collections.py
#2778241945	@aucahuasi	Validate set['id'] is required	✅ Added validation warning; drops in autofix mode (no auto-generation - user must provide meaningful IDs)
#2683481643	@mj3cheun	Type string coercion pointless	✅ Removed str(collection_type) - now continues/fails since coercion won't produce valid types

MEDIUM Priority - Fixed

Comment	Author	Issue	Resolution
#2683617536	@mj3cheun	Use Chain.from_json for GFQL parsing	✅ Now uses _wrap_gfql_expr which handles Chain/AST conversion canonically
#2683627736	@mj3cheun	Dead if statement (intersection type check)	✅ Actually NOT dead - validates expr.type vs collection.type mismatch; kept for safety

LOW Priority - Fixed

Comment	Author	Issue	Resolution
#2683343387	@mj3cheun	Redundant normalize_validation_params call	Keep for API safety - ensures consistent param handling regardless of caller
#2683393920	@mj3cheun	Inline _coerce_collection_list	✅ Inlined into _parse_collections_input

Still Open / Deferred

Comment	Author	Issue	Status
#2683317737	@mj3cheun	Auto-detect pre-encoded strings	Deferred - encode param was removed; always canonicalizes now
#2669705397	@lmeyerov	"back out" plot-time validation	Kept - acts as safety net for bypassing .collections() method

---

Key Design Decisions

1. No ID auto-generation: Server requires IDs but client should not generate arbitrary ones like set_0. Collections without IDs are dropped in autofix mode with a warning. Users must provide meaningful IDs.

2. Precise exception handling: Changed except Exception to except (TypeError, ValueError, GFQLValidationError) to avoid masking bugs.

3. Cross-validation for intersections: New _validate_intersection_references() ensures intersection sets reference actual collection IDs. Dangling references are caught before hitting the backend.

4. GFQL parsing consolidation: _normalize_gfql_ops now delegates to _wrap_gfql_expr from collections.py as the canonical implementation.

5. Proper type handling: Used dict(entry) to convert TypedDicts to plain dicts at runtime for uniform handling, avoiding unchecked static casts.

---

CI Status: All jobs green (manual workflow_dispatch run 21793451172) - python-lint-types 3.8-3.14 ✅, all test suites ✅

[aucahuasi]

Thanks for fixing the previous issue, here are some others that caught my attention:

https://github.com/graphistry/pygraphistry/pull/874/changes#r2789010065
https://github.com/graphistry/pygraphistry/pull/874/changes#r2789125932

[lmeyerov]

Thanks for the thorough reviews! All comments have been addressed:

Consolidation & Architecture:

• Moved GFQL normalization to compute/ast.py:normalize_gfql_to_wire() as canonical implementation
• Clean import graph: models → compute → helpers/validation
• collections.py and validate_collections.py both call compute/ast

Validation Improvements:

• Intersection DAG validation: supports intersections-of-intersections, detects self-references and cycles
• Added AssertionError to exception handling (AST from_json methods use bare asserts)
• Empty sets list validation
• Cross-validation of intersection set ID references
• Auto-generate kebab-case IDs in autofix mode (set-0, intersection-1)

Code Quality:

• Removed dead code after return statements
• Proper type handling with dict(entry) instead of unchecked casts
• 22 tests covering all validation paths

CI is green, ready for merge.