Conversation
Co-authored-by: timurbazhirov <721112+timurbazhirov@users.noreply.github.com>
Co-authored-by: timurbazhirov <721112+timurbazhirov@users.noreply.github.com>
src/py/mat3ra/__init__.py
Outdated
| """mat3ra namespace package.""" | ||
| import pkgutil | ||
|
|
||
| __path__ = pkgutil.extend_path(__path__, __name__) |
There was a problem hiding this comment.
We need this in every py pacakge we have so that we can install from local codebase when we run tests, and have installed packages resolved too. did this in Made, Mode
|
|
||
| @property | ||
| def extra_data_key(self) -> str: | ||
| name_str = self.name.value if hasattr(self.name, 'value') else str(self.name) |
tests/js/template.test.ts
Outdated
| [ContextProviderNameEnum.QENEBInputDataManager]: mockConfig, | ||
| [ContextProviderNameEnum.VASPInputDataManager]: mockConfig, | ||
| [ContextProviderNameEnum.VASPNEBInputDataManager]: mockConfig, | ||
| [ContextProviderNameEnum.NWChemInputDataManager]: mockConfig, |
There was a problem hiding this comment.
We shouldn't need this and above file changes
tests/py/test_template.py
Outdated
| assert template.content == "&CONTROL\n/" | ||
| assert template.applicationName == "espresso" | ||
| assert template.executableName == "pw.x" | ||
| assert len(template.context_providers) == 1 |
There was a problem hiding this comment.
We can use assert deep almost equal
| json_schema={"type": "object"}, | ||
| ) | ||
| assert provider.is_using_jinja_variables is True | ||
| assert provider.json_schema == {"type": "object"} |
There was a problem hiding this comment.
Seems like this file is not needed
tests/py/test_integration.py
Outdated
| assert app_restored.name == app.name | ||
| assert exe_restored.name == executable.name | ||
| assert tmpl_restored.name == template.name | ||
| assert flv_restored.name == flavor.name |
There was a problem hiding this comment.
Pull request overview
This PR adds a Python implementation of the ADE (Application DEfinitions) codebase alongside the existing JavaScript/TypeScript implementation, enabling Python developers to work with application definitions, templates, executables, and flavors.
- Implements Python versions of core ADE classes:
Application,Executable,Flavor,Template, andContextProvider - Adds comprehensive test coverage for all new Python classes using pytest
- Updates project configuration to support both JavaScript and Python development workflows
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/py/mat3ra/ade/init.py | Replaces sample function with proper module exports for ADE classes |
| src/py/mat3ra/init.py | Configures mat3ra as a namespace package using pkgutil |
| src/py/mat3ra/ade/application.py | Implements Application class with material usage detection and short name handling |
| src/py/mat3ra/ade/executable.py | Implements Executable class for application executables |
| src/py/mat3ra/ade/flavor.py | Implements Flavor and FlavorInput classes for executable flavors |
| src/py/mat3ra/ade/template.py | Implements Template class with Jinja2 rendering and context provider support |
| src/py/mat3ra/ade/context/context_provider.py | Implements ContextProvider base class for template context management |
| src/py/mat3ra/ade/context/jinja_context_provider.py | Implements JinjaContextProvider extending ContextProvider |
| src/py/mat3ra/ade/context/json_schema_data_provider.py | Implements JSONSchemaDataProvider with JSON schema support |
| tests/py/test_template.py | Adds comprehensive tests for Template class functionality |
| tests/py/test_sample.py | Removes obsolete sample test file |
| tests/py/test_flavor.py | Adds tests for Flavor and FlavorInput classes |
| tests/py/test_executable.py | Adds tests for Executable class |
| tests/py/test_context_provider.py | Adds tests for ContextProvider functionality |
| tests/py/test_application.py | Adds tests for Application class including material usage checks |
| pyproject.toml | Adds Python dependencies (pydantic, jinja2, mat3ra packages) and test requirements |
| README.md | Documents Python installation, development commands, and testing instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/py/mat3ra/ade/template.py
Outdated
| print(f"Template is not compiled: {e}") | ||
| print(f"content: {self.content}") | ||
| print(f"_clean_rendering_context: {self._clean_rendering_context(rendering_context)}") |
There was a problem hiding this comment.
Using print() for error logging is not a best practice. Consider using Python's logging module instead for proper error handling and control over log levels. This would allow users to configure logging behavior and integrate with existing logging infrastructure. Example:
import logging
logger = logging.getLogger(__name__)
# Then in the except block:
logger.error(f"Template is not compiled: {e}")
logger.debug(f"content: {self.content}")
logger.debug(f"_clean_rendering_context: {self._clean_rendering_context(rendering_context)}")| def _get_rendering_context( | ||
| self, external_context: Optional[Dict[str, Any]] = None | ||
| ) -> Dict[str, Any]: | ||
| provider_context = external_context or {} |
There was a problem hiding this comment.
The provider_context parameter is defined but never used in this method. It's assigned at line 72 but not passed to _get_data_from_providers_for_rendering_context() at line 75. This suggests either the parameter should be removed, or it should be passed to the method call if it's meant to provide context to providers.
tests/py/test_template.py
Outdated
|
|
||
|
|
||
| def test_template_to_dict(): | ||
| config = { |
pyproject.toml
Outdated
| "pydantic>=2.0", | ||
| "mat3ra-esse", | ||
| "mat3ra-code @ git+https://github.com/Exabyte-io/code.git@c2ddf37759be80a50415af2a68096ab82c9683e5", | ||
| "jinja2>=3.0" |
There was a problem hiding this comment.
Should be done through mat3ra-utils[extra]
src/py/mat3ra/ade/template.py
Outdated
| cleaned_context = self._clean_rendering_context(rendering_context) | ||
| rendered = template.render(cleaned_context) | ||
| self.rendered = rendered or self.content | ||
| except TemplateError as e: |
There was a problem hiding this comment.
Should be render_jinja_with_error_handling in utils
| def context_providers(self) -> List[ContextProvider]: | ||
| return self.contextProviders | ||
|
|
||
| @context_providers.setter |
There was a problem hiding this comment.
Let's have a separate task to deal with case conversion
There was a problem hiding this comment.
src/py/mat3ra/ade/template.py
Outdated
| cleaned.pop("job", None) | ||
| return cleaned | ||
|
|
||
| def _get_data_from_providers_for_rendering_context( |
There was a problem hiding this comment.
We should make it non-private and test
src/py/mat3ra/ade/template.py
Outdated
| result: Dict[str, Any] = {} | ||
| for provider in self.context_providers: | ||
| context = provider.yield_data_for_rendering(provider_context) | ||
| for key, value in context.items(): |
There was a problem hiding this comment.
Let's isolate to context provider itself
|
Coverage after merging feature/SOF-7755 into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
No description provided.