refactor: zero-hardcode settings-driven rebuild - #6
Conversation
- central MATGRAPH_* settings (models/cache/auth/ga/hull/graphql) - honest band_gap (None) + provenance/seed/validation/parquet - hashed auth (sha256+scopes/ttl), WAL cache, layered config - pluggable models, batched SDK (DataFrame/async/from_structures) - CLI --seed/--allowed-elements, REST /v1/predict + /health - tooling ruff/mypy/pytest + 9 tests + Docker
|
|
![trinetra-bote[bot]](https://avatars.githubusercontent.com/in/4535959?s=60&v=4){kind=small}
| COPY pyproject.toml README.md ./ | ||
| COPY matgraph ./matgraph | ||
| RUN pip install --no-cache-dir -e . | ||
| EXPOSE 8000 |
There was a problem hiding this comment.
For production deployments, it's generally recommended to install packages in non-editable mode (pip install --no-cache-dir .) to ensure a cleaner and more predictable build. Editable installs (-e .) are more suited for development. Consider switching this for a production-optimized Dockerfile if this image is intended for deployment.
| json.dump(keys, fh, indent=4) | ||
| try: | ||
| f.chmod(0o600) | ||
| except Exception: |
There was a problem hiding this comment.
The try...except Exception block for f.chmod(0o600) is quite broad. While chmod can fail on certain filesystems (e.g., FAT32, network shares), it's good practice to either log the specific exception (e.g., logging.warning("Could not set permissions on auth keys file: %s", e)) or catch more specific exceptions if known, to avoid masking other potential issues.
|
|
||
| return False | ||
| # legacy: if keys contain plaintext key directly, migrate check | ||
| if api_key in keys: |
There was a problem hiding this comment.
The if api_key in keys: block allows for legacy plaintext API keys to still be valid. While this is a good temporary measure for migration, it means plaintext keys are still being stored and checked, which undermines the security improvement of hashing. I recommend adding a deprecation warning for these legacy keys and planning for their eventual removal after a suitable migration period. Users should be prompted to regenerate their keys.
| json.dump(config, fh, indent=4) | ||
| try: | ||
| f.chmod(0o600) | ||
| except Exception: |
There was a problem hiding this comment.
Similar to matgraph/auth.py, the try...except Exception block for f.chmod(0o600) is quite broad. Consider logging the specific exception or catching more specific exceptions if known, to avoid masking other potential issues.
| new_structure.replace_species({elem_out: elem_in}) | ||
| except Exception as e: | ||
| raise ValidationError(f"Substitution failed (incompatible species): {e}") | ||
|
|
There was a problem hiding this comment.
The client-side filtering for min_gap, max_gap, and crystal_system is performed after fetch_materials_data. Since fetch_materials_data already accepts these parameters and passes them to MPRester.materials.summary.search, this client-side filtering is redundant. It's harmless but could be removed for clarity and efficiency if the fetch_materials_data function is guaranteed to apply these filters comprehensively.
| except Exception as e: | ||
| raise ValidationError(f"Invalid element symbol: {e}") | ||
| if elem_out == elem_in: | ||
| raise ValidationError("element_out and element_in must differ") |
There was a problem hiding this comment.
The comment logger.debug("Substitution validation passed for %s: %s->%s", formula, elem_out, elem_in) is good, but the except Exception block below it for charge neutrality currently just passes without logging a warning. If the intent is to warn the user about potential charge neutrality issues without blocking the substitution, a logger.warning statement should be added here.
| crystal_system: typing.Optional[str] = None, | ||
| model: typing.Optional[str] = "cgcnn", | ||
| limit: typing.Optional[int] = 10 | ||
| limit: typing.Optional[int] = None |
There was a problem hiding this comment.
The model parameter defaults to "cgcnn" in the GraphQL schema, but run_pipeline normalizes "cgcnn" to "m3gnet". To avoid confusion and align with the internal model naming, it might be clearer to default this parameter to "m3gnet" directly in the GraphQL schema.
| model: str = "m3gnet" | ||
| limit: typing.Optional[int] = None | ||
|
|
||
| @app.post("/v1/predict", dependencies=[Depends(get_api_key)]) |
There was a problem hiding this comment.
The API key retrieval os.getenv("MP_API_KEY") or _cfg_get() or "" in the rest_predict endpoint could be simplified by directly using matgraph.config.get_api_key(), which already handles the environment variable and config file precedence. This would reduce duplication and ensure consistent API key lookup logic.
| gap_errors, form_errors = [], [] | ||
|
|
||
| def predict(self, formula: str, model: str = "m3gnet", min_gap: Optional[float] = None, max_gap: Optional[float] = None, crystal_system: Optional[str] = None, seed: Optional[int] = None, as_frame: Optional[str] = None) -> Union[List[Dict[str, Any]], Any]: | ||
| """Predict with optional DataFrame return: as_frame='pandas'|'polars'.""" |
There was a problem hiding this comment.
The ValueError raised when the API key is missing could be replaced with a more specific ConfigError from matgraph.exceptions for better structured error handling and clarity for SDK users.
| prov = _provenance(seed=seed) | ||
| out = [] | ||
| for s in structures: | ||
| feats = extract_features(s) |
There was a problem hiding this comment.
The max_workers parameter for ThreadPoolExecutor in predict_many is currently hardcoded to 4. For a more flexible SDK, consider making this configurable via a parameter to predict_many or through a setting in matgraph.settings, allowing users to adjust concurrency based on their environment and workload.
- BREAKING: only m3gnet ships (cgcnn/megnet removed, Blocked with ValidationError) - remove hardcoded confidence 0.92 -> uncertainty=None + note - predicted_band_gap Optional + cache key includes model_version+git_sha - README: honest M3GNet-only, heuristic discovery, reproducible cache, hashed keys - split god module: data/models/discovery/simulation/dft/properties/api packages (core.py shim) - pyproject 2.0.0 + optional[ml,dft,api,ui,all]
|
|
| else: | ||
| info = keys.get(h) | ||
| if not info or not info.get("active", False): | ||
| return False |
There was a problem hiding this comment.
The logic if api_key in keys: allows legacy plaintext API keys to be directly present in the ~/.matgraph_keys.json file. While this provides a smooth migration path, it means that if an old keys.json file containing plaintext keys is compromised, those keys would still be valid. Consider adding a strong warning during migration or a mechanism to automatically hash and update legacy keys to improve security posture.
| from matgraph.ga import CrystalGA | ||
| ga = CrystalGA(base_formula=formula, api_key=self.api_key, population_size=population_size) | ||
| return ga.run(generations=generations) | ||
| return inverse_design(api_key=self.api_key, min_gap=min_gap, max_gap=max_gap, crystal_system=crystal_system, exclude_elements=exclude_elements, include_elements=include_elements, limit=limit) |
There was a problem hiding this comment.
The formation_energy_mae currently defaults to 0.0 if form_errors is empty. This can be misleading if no valid formation energy predictions were available (e.g., all true_form_energy or predicted_form_energy were None). An MAE of 0.0 implies perfect prediction, which is incorrect in such cases. Consider returning None for formation_energy_mae as well when form_errors is empty, similar to band_gap_mae.
1 participant
Central MATGRAPH_* settings, honest band_gap, provenance/seed, hashed auth, WAL cache, pluggable models, batched SDK, CLI --seed, REST /v1/predict, tooling + 9 tests + Docker