fix#13
Conversation
…or-project-showcase Add GitHub Pages-ready interactive void viewer
Bender1011001
merged commit a2691a4
into
codex/set-up-github-pages-for-project-showcase
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| def retry_with_backoff( | ||
| max_retries: int = 3, | ||
| base_delay: float = 1.0, | ||
| backoff_factor: float = 2.0, | ||
| jitter: bool = True, | ||
| raise_on_permanent: bool = True, | ||
| expected_exceptions: tuple = (RequestException,), | ||
| circuit_breaker: Optional[CircuitBreaker] = None, | ||
| ) -> Callable: | ||
| """Decorator for exponential backoff retry with jitter and error categorization.""" | ||
|
|
||
| def decorator(func: Callable) -> Callable: | ||
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs) -> Any: | ||
| last_exception = None | ||
| for attempt in range(max_retries + 1): | ||
| try: | ||
| if circuit_breaker: | ||
| return circuit_breaker.call(func, *args, **kwargs) | ||
| return func(*args, **kwargs) |
There was a problem hiding this comment.
The decorator assumes max_retries, base_delay, and circuit_breaker are numeric values, iterating with range(max_retries + 1) and calling circuit_breaker.call(...) directly. However, download_with_retry registers the decorator with lambdas to read instance attributes, so at runtime max_retries and circuit_breaker are callable objects rather than numbers or a CircuitBreaker. The first call to the wrapper will raise TypeError: unsupported operand type(s) for +: 'function' and 'int' and never reach the download logic. The decorator needs to evaluate callables per invocation (e.g., value = max_retries(self) when callable) before using them.
Useful? React with 👍 / 👎.
| def ensure_dns(hosts: list[str], timeout: int = 5, max_retries: int = 3) -> None: | ||
| """Preflight DNS resolution check for hosts.""" | ||
| for host in hosts: | ||
| for attempt in range(max_retries): | ||
| try: | ||
| socket.getaddrinfo(host, 443, timeout=timeout) | ||
| logger.info(f"✓ DNS OK: {host}") |
There was a problem hiding this comment.
ensure_dns calls socket.getaddrinfo(host, 443, timeout=timeout). The stdlib function does not accept a timeout keyword, so the very first host immediately raises TypeError: getaddrinfo() got an unexpected keyword argument 'timeout', preventing any DNS preflight or retry logic from executing. The call should use socket.setdefaulttimeout or wrap socket.getaddrinfo in socket.create_connection rather than passing a non‑existent keyword.
Useful? React with 👍 / 👎.
| output_path: Path, | ||
| desc: str = None, | ||
| auth_service: Optional[str] = None, | ||
| chunk_size: int = 8192, | ||
| expected_size: Optional[int] = None, | ||
| checksum: Optional[str] = None, | ||
| ) -> bool: | ||
| """Download with full resilience features.""" | ||
| try: | ||
| headers = self._get_headers(auth_service) | ||
| resp = self.session.get( | ||
| url, | ||
| headers=headers, | ||
| stream=True, | ||
| timeout=self.timeout, | ||
| allow_redirects=True, | ||
| ) | ||
| resp.raise_for_status() | ||
|
|
||
| total_size = int(resp.headers.get("content-length", 0)) | ||
| if expected_size and total_size != expected_size: | ||
| logger.warning(f"Size mismatch: expected {expected_size}, got {total_size}") | ||
|
|
||
| output_path.parent.mkdir(parents=True, exist_ok=True) | ||
| if output_path.exists(): | ||
| logger.info(f"File exists, skipping: {output_path}") | ||
| return self._validate_file(output_path, checksum) | ||
|
|
||
| start_time = time.time() | ||
| downloaded = 0 | ||
| last_throttle_time = start_time | ||
| throttle_interval = chunk_size / self.bandwidth_throttle if self.bandwidth_throttle else 0 | ||
|
|
||
| with open(output_path, "wb") as f, tqdm( | ||
| total=total_size, | ||
| unit="B", | ||
| unit_scale=True, |
There was a problem hiding this comment.
download_with_retry and _validate_file reference Path (type hints and path operations), tqdm (progress bar) and hashlib (checksum) but the module only imports functools, logging, etc. Without importing pathlib.Path, tqdm or hashlib, importing this module or calling these functions will raise NameError before any download happens. Add the missing imports so the resilient downloader can be used.
Useful? React with 👍 / 👎.
| def generate_graph(self, template_params: Dict, output_path: str) -> str: | ||
| """ | ||
| Generate SNAP graph with substituted parameters. | ||
|
|
||
| :param template_params: Dict of params to substitute (e.g., {'SUBSWATH': 'IW1'}). | ||
| :param output_path: Output XML path. | ||
| :return: Path to generated graph. | ||
| """ | ||
| # Map to template keys (upper case with _) | ||
| subst_dict = {k.upper(): str(v) for k, v in template_params.items()} | ||
| subst_dict['MASTER'] = template_params.get('input_file', '${master}') | ||
| subst_dict['SLAVE'] = template_params.get('input_file', '${slave}') # Update for slave in full process | ||
| subst_dict['OUTPUT'] = f"{template_params.get('output_paths', 'interferogram')}.tif" |
There was a problem hiding this comment.
generate_graph substitutes both ${MASTER} and ${SLAVE} with template_params.get('input_file', ...), so when process_interferogram passes parameters derived only from the master scene the generated graph contains the same SAFE path for both nodes. Even though the GPT command later passes -Pslave=..., the graph file already hardcodes the master file twice, causing the interferogram to use identical inputs and produce invalid results. The substitution should accept separate master and slave paths (e.g., parameters master_safe/slave_safe).
Useful? React with 👍 / 👎.
| # Run via GPT (assume gpt in PATH; add snap install check if needed) | ||
| cmd = [ | ||
| 'gpt', str(graph_path), | ||
| '-Pmaster=' + master_safe, | ||
| '-Pslave=' + slave_safe, | ||
| '-Poutput=' + str(output_dir / f"{params['output_paths']}.tif") | ||
| ] | ||
| try: | ||
| result = subprocess.run(cmd, capture_output=True, text=True, check=True) |
There was a problem hiding this comment.
process_interferogram builds the command list and executes subprocess.run(...), but the module never imports subprocess. Any call to this helper therefore crashes with NameError: name 'subprocess' is not defined before SNAP can be launched. Add import subprocess near the other imports.
Useful? React with 👍 / 👎.
…or-project-showcase Merge pull request #13 from Bender1011001/master
…or-project-showcase Merge pull request #13 from Bender1011001/master
1 participant
No description provided.