Skip to content

Conversation

@yanchengnv
Copy link
Collaborator

Fixes # .

Description

This PR introduces federated object exchange - a new way to develop FL applications.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

96 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (33)

  1. nvflare/fox/examples/np/algos/utils.py, line 65-67 (link)

    style: Function modifies input dictionary in-place without clear indication. Consider returning modified copy or documenting the in-place behavior. Should this function modify the input dictionary in-place or return a new dictionary?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. nvflare/fox/examples/np/algos/utils.py, line 57-62 (link)

    style: Function modifies to_model parameter in-place without clear indication. Consider documenting this behavior or returning the modified dictionary.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  3. nvflare/fox/sys/backend.py, line 52 (link)

    logic: Context is removed from kwargs but what happens if CollabMethodArgName.CONTEXT key doesn't exist? Should this use kwargs.pop() with a default value or check for key existence first?

  4. nvflare/fox/sys/backend.py, line 80-82 (link)

    logic: Raising TimeoutError for non-Message reply seems incorrect - this appears to be an internal communication error rather than a timeout

  5. nvflare/fox/examples/pt/utils.py, line 54-55 (link)

    style: The div function modifies the input dictionary in-place, which could cause unexpected side effects if the caller expects the original model to remain unchanged. Should this function create a copy of the model dictionary to avoid mutating the input?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  6. nvflare/fox/examples/pt/utils.py, line 25 (link)

    style: Using torch.Tensor() constructor can be inefficient and may not preserve data types - consider using torch.tensor() instead

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  7. nvflare/fox/examples/np/algos/swarm.py, line 92-93 (link)

    style: Using bare except: catches all exceptions including system exits and keyboard interrupts. Consider catching specific exceptions or Exception

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  8. nvflare/fox/sys/adaptor.py, line 32-33 (link)

    logic: collab_obj_ids is reassigned but self.collab_obj_ids uses the parameter value before reassignment

  9. nvflare/fox/sys/adaptor.py, line 97-99 (link)

    logic: filter_ids type validation missing - could fail if not a list

  10. nvflare/fox/utils/tensor_receiver.py, line 46 (link)

    logic: error message includes model variable which contains downloaded result, not original reference

  11. nvflare/fox/sim/simulator.py, line 60-70 (link)

    style: Uses copy.deepcopy() to clone client app instances, which may cause issues if the client app contains non-serializable objects or references. Are there constraints on what types of objects can be stored in client apps that would make deep copying problematic?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  12. nvflare/fox/sim/simulator.py, line 186 (link)

    style: Thread executor shutdown with cancel_futures=True may interrupt ongoing operations abruptly without proper cleanup. Should there be a graceful shutdown period before canceling futures?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  13. nvflare/fox/examples/pt/pt_avg_stream.py, line 80 (link)

    style: Group call executes asynchronously - consider adding timeout or wait mechanism to ensure all clients respond before proceeding to aggregation

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  14. nvflare/fox/examples/pt/pt_avg_stream.py, line 82-84 (link)

    logic: Returning None when no clients respond could cause issues downstream - consider returning the original model or raising an exception. Should the algorithm continue with the current model if no clients respond, or should it fail fast?

  15. nvflare/fox/api/app.py, line 106-107 (link)

    logic: adding original filter f to managed objects but appending wrapped filter_obj to chain - should add filter_obj to managed objects instead

  16. nvflare/fox/api/app.py, line 394-396 (link)

    style: inconsistent return pattern - ServerApp.get_children() returns empty list when no children, but this returns None. Should this return an empty list for consistency with the parent class pattern?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  17. nvflare/fox/api/facade.py, line 30 (link)

    style: Class name should follow PascalCase convention

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  18. nvflare/fox/api/proxy.py, line 230-232 (link)

    logic: Exception handling returns the exception object instead of raising it, which could lead to silent failures if callers don't check return types. Should the exception be re-raised here, or is returning it the intended behavior for the FOX API design?

  19. nvflare/fox/examples/np/algos/strategies/avg_para.py, line 52 (link)

    logic: Variable total is initialized but not used in the calculation - only accumulated results are used for averaging

  20. nvflare/fox/sys/controller.py, line 84 (link)

    logic: should validate that server_obj is not None before creating ServerApp

  21. nvflare/fox/api/group.py, line 102-104 (link)

    style: Documentation mentions ResultQueue but the actual return type is waiter.results - consider updating the docstring to match the implementation

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  22. nvflare/fox/api/utils.py, line 64-69 (link)

    logic: The argument count validation doesn't account for functions with default parameters or *args/**kwargs. This could incorrectly reject valid calls to functions with flexible parameter lists. Should this validation handle functions with default parameters, *args, or **kwargs that might accept more arguments than the basic parameter count suggests?

  23. nvflare/fox/sys/downloader.py, line 89 (link)

    syntax: Error message inconsistency: says 'SysBackend' but should be 'FlareBackend' to match the actual check

  24. nvflare/fox/sys/downloader.py, line 108 (link)

    syntax: Error message inconsistency: says 'SysBackend' but should be 'FlareBackend' to match the actual check

  25. nvflare/fox/sys/downloader.py, line 129 (link)

    syntax: Error message inconsistency: says 'SysBackend' but should be 'FlareBackend' to match the actual check

  26. nvflare/fox/examples/pt/pt_np.py, line 103-105 (link)

    logic: Inconsistent return type - returns 0 on abort but tuple otherwise

  27. nvflare/fox/api/ctx.py, line 81-82 (link)

    logic: Context manager should set context on entry

  28. nvflare/fox/examples/pt/pt_avg_mixed.py, line 61 (link)

    logic: Early return with None values could cause issues in subsequent rounds if _do_one_round returns None. What should happen when a round fails and returns None models?

  29. nvflare/fox/examples/pt/pt_avg_mixed.py, line 153-155 (link)

    logic: Returns literal 0 instead of expected tuple when training is aborted

  30. nvflare/fox/api/gcc.py, line 48-49 (link)

    logic: Queue full check happens after incrementing counter but queue is modified on line 50 regardless - this could lead to inconsistent state if an exception occurs

  31. nvflare/fox/api/gcc.py, line 193 (link)

    logic: Context is restored in success path but not in exception path - original context may be lost if set_call_context(ctx) succeeds but subsequent operations fail

  32. nvflare/fox/examples/np/algos/avg_stream.py, line 121 (link)

    logic: Returning 0 instead of a proper tuple (result, model_type) breaks the expected return format.

  33. nvflare/fox/api/dec.py, line 212-214 (link)

    style: The adjust_kwargs function modifies the kwargs dictionary in-place, which could have side effects if the same kwargs is used elsewhere. Consider creating a copy or documenting this behavior.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

96 files reviewed, 33 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. nvflare/fox/sys/executor.py, line 78 (link)

    logic: self.client_app is None at this point (set to None on line 65), so getattr(self.client_app, ...) will fail with AttributeError

96 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (3)

  1. nvflare/fox/api/app.py, line 216 (link)

    logic: Regex pattern requires at least 2 characters (letter + alphanumeric), rejecting single-character names like "x" or "A" which may be valid use cases

  2. nvflare/fox/sim/simulator.py, line 77 (link)

    style: Deep copy may fail for objects with non-copyable attributes (locks, threads, file handles). Consider wrapping in try/except to provide clearer error message directing users to implement make_client_app

  3. nvflare/fox/api/group.py, line 164 (link)

    logic: Race condition: in_sending_count read without lock, value could change between check and inc_sending() call, potentially exceeding max_parallel limit

96 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (3)

  1. nvflare/fox/sys/controller.py, line 79 (link)

    style: Thread pool executor created without explicit thread_name_prefix, making debugging harder. Consider adding prefix like "FOX-Controller-" for easier log analysis.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. nvflare/fox/api/group.py, line 163-175 (link)

    style: Busy-wait loop checking in_sending_count with 0.1s sleep. This works but could be more efficient with condition variable or semaphore for signaling when slots become available.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  3. nvflare/fox/api/app.py, line 216-218 (link)

    style: Pattern allows alphanumeric and underscores but starts with letter. The error message could be more specific about what characters are allowed.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

96 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (6)

  1. nvflare/fox/api/ctx.py, line 84-88 (link)

    logic: Context not restored properly if parent_ctx is None. Line 88 sets context to None which could cause issues if another context was set by a different thread between __enter__ and __exit__.

  2. nvflare/fox/api/group.py, line 163-178 (link)

    style: Busy-wait loop with 0.1s sleep - inefficient CPU usage when max parallel limit reached. Consider using a condition variable or semaphore instead of polling waiter.in_sending_count.

  3. nvflare/fox/api/group.py, line 189-196 (link)

    style: Another busy-wait loop - same performance concern as above. Repeatedly checking abort_signal.triggered and waiting with 0.1s sleep is inefficient.

  4. nvflare/fox/sys/controller.py, line 260-261 (link)

    style: Thread executor shutdown uses cancel_futures=True which may cancel in-flight remote calls abruptly. Verify this is the intended behavior during controller shutdown.

  5. nvflare/fox/sys/executor.py, line 100 (link)

    style: Same as controller - verify that cancel_futures=True won't cause issues with ongoing remote calls during client shutdown.

  6. nvflare/private/fed/server/server_engine.py, line 450-454 (link)

    style: Added fallback to run_manager.cell when self.cell is None. Check that all callers handle None return, since both could potentially be None.

96 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. nvflare/fox/api/gcc.py, line 91 (link)

    logic: Condition should be initialized with a lock. Currently initialized without arguments, which creates an internal lock. However, self.lock on line 90 is never used.

96 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. nvflare/fox/api/gcc.py, line 93-95 (link)

    style: lock must be initialized before being used in condition

    The self.lock is used to initialize self.sending_decreased condition variable on line 91, but self.lock is only defined on line 90. While this works in Python due to execution order, it would be clearer to initialize the lock before using it in the Condition:

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. nvflare/fox/api/gcc.py, line 89-91 (link)

    style: in_sending_count initialization should use the lock

    The counter is accessed from multiple threads and should be initialized with the lock held for consistency, or at minimum document that initialization happens before any concurrent access begins.

96 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (12)

  1. nvflare/fox/examples/__init__.py, line 25 (link)

    style: Consider adding type hint for make_recipe_f parameter

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. nvflare/fox/examples/pt/filters.py, line 38 (link)

    style: redundant assignment - num_receivers is already assigned on line 34

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  3. nvflare/fox/examples/np/algos/strategies/avg_intime.py, line 83 (link)

    style: The gcc parameter is not used in this callback function

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  4. nvflare/fox/examples/np/algos/strategies/avg_h.py, line 48-49 (link)

    style: Variable num_results is calculated but len(results) is used again in the return statement

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  5. nvflare/fox/sim/simulator.py, line 64 (link)

    syntax: Typo: 'nme' should be 'name'

  6. nvflare/fox/examples/np/algos/strategies/cyclic.py, line 65-67 (link)

    logic: Early return on None breaks the round but continues to next round - verify this is intended behavior vs stopping all training. Should a failed client training stop the entire cyclic process or just skip to the next round?

  7. nvflare/fox/examples/pt/pt_avg_filter.py, line 71 (link)

    logic: returning 0 when training is aborted may cause issues since the aggregation logic expects model weights (dict), not an integer

    should this return None or an empty dict to match the expected return type?

  8. nvflare/fox/examples/np/algos/filters.py, line 34 (link)

    style: Inconsistent logging approach - using print() instead of self.logger for user feedback

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  9. nvflare/fox/examples/np/algos/filters.py, line 49 (link)

    style: Unnecessary super().init() call since no parent class is explicitly defined

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  10. nvflare/fox/api/app.py, line 80-81 (link)

    logic: Should check if _me is None before accessing backend to prevent AttributeError

  11. nvflare/fox/api/app.py, line 360-361 (link)

    logic: Base App.has_children() always returns True but get_children() returns empty list - this inconsistency could cause logic errors in hierarchy traversal

  12. nvflare/fox/examples/np/algos/avg_stream.py, line 65 (link)

    style: Using /tmp directory for temporary files may not be portable across all platforms and could cause permissions issues in containerized environments. Should this use a more robust temporary directory approach like tempfile.mkdtemp() for better cross-platform compatibility?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

96 files reviewed, 12 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (3)

  1. nvflare/fox/api/gcc.py, line 38-55 (link)

    logic: Race condition: num_whole_items_received is checked and modified without proper synchronization. Multiple threads can simultaneously check line 49 and then both increment line 54, violating the limit constraint. Wrap the entire method in with self.lock:

  2. nvflare/fox/sys/utils.py, line 124-126 (link)

    style: Exception details are suppressed - only exception type is returned. This makes debugging difficult. Include exception message for troubleshooting

  3. nvflare/fox/api/proxy.py, line 228-233 (link)

    style: Backend exception handler is called but may raise the exception again. If handle_exception re-raises after logging, this code still returns the exception as a value, masking the actual control flow. Does backend.handle_exception always re-raise, or does it sometimes return normally?

96 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (21)

  1. nvflare/fox/sys/adaptor.py, line 32-33 (link)

    style: Reassigning the parameter creates potential confusion - use self.collab_obj_ids = collab_obj_ids or [] instead

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. nvflare/recipe/spec.py, line 112-118 (link)

    style: Empty docstring return section - consider removing the 'Returns:' line since the method returns None

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  3. nvflare/fox/sim/backend.py, line 132 (link)

    logic: calling call_target within group context creates potential for nested waiter creation and double timeout handling. Should group calls have their own timeout management independent of the individual call timeout?

  4. nvflare/fox/sim/backend.py, line 66 (link)

    logic: setting waiter.result to RunAborted bypasses normal exception handling flow, but waiter may already have a result from _run_func

  5. nvflare/fox/examples/pt/pt_avg_filter.py, line 32 (link)

    style: Class name mismatch - filename suggests 'filter' functionality but class is named 'PTFedAvgStream' (which doesn't match the class name 'PTFedAvg'). Is this intended to demonstrate filtering capabilities or just federated averaging?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  6. nvflare/fox/examples/pt/pt_avg_filter.py, line 31 (link)

    style: The timeout parameter is stored but never used in the implementation

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  7. nvflare/fox/api/proxy.py, line 228-238 (link)

    style: Unclear exception control flow - if backend.handle_exception() re-raises the original exception, this code will still return it as a value, potentially masking the intended behavior. Should the backend exception handler be able to override the return-exception behavior?

  8. nvflare/fox/api/app.py, line 392-395 (link)

    style: Using assertion for runtime validation in production code - consider replacing with proper exception handling

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  9. nvflare/fox/examples/np/algos/strategies/avg_para.py, line 52 (link)

    logic: Variable total is declared but never used in the averaging calculation

  10. nvflare/fox/api/group.py, line 184-187 (link)

    logic: exception handling incomplete: the exception is logged and handled by backend but not re-raised, potentially masking critical errors

  11. nvflare/fox/examples/np/fed_avg_intime.py, line 39-43 (link)

    logic: Server timeout (5s) is shorter than client timeout (8s). This could cause server-side timeouts while clients are still processing. Is this intentional to test timeout handling, or should server timeout be longer than client timeout?

  12. nvflare/fox/sys/utils.py, line 124-126 (link)

    style: Exception details are suppressed - only exception type is returned to caller, making remote debugging difficult. Is this intentional to prevent information leakage, or should more detailed error information be provided for debugging?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  13. nvflare/fox/sys/recipe.py, line 180 (link)

    style: Using assert for type validation in production code can be disabled with -O flag, potentially causing runtime errors.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  14. nvflare/fox/examples/pt/pt_avg_stream2.py, line 82-88 (link)

    logic: Count tracking assumes each tensor chunk represents one complete contribution per site, but streaming may deliver tensors in variable-sized chunks. Should the counting logic account for partial tensor chunks from streaming, or does each chunk represent a complete tensor contribution?

  15. nvflare/fox/sys/downloader.py, line 25-26 (link)

    logic: Import path ...app_opt.pt.tensor_downloader appears incorrect - this module likely doesn't exist in the repository structure

  16. nvflare/fox/examples/np/algos/strategies/avg_para_tc.py, line 50-51 (link)

    logic: Re-raising exception immediately terminates the round, preventing other successful clients from contributing. Should the algorithm continue with partial results when some clients fail, rather than failing the entire round?

  17. nvflare/fox/examples/np/algos/swarm.py, line 102-105 (link)

    logic: Random client selection could pick the same client that's currently executing, causing infinite self-loops. Consider excluding the current client from selection. Should the next client selection exclude the current client to prevent self-loops?

  18. nvflare/fox/examples/np/algos/filters.py, line 40 (link)

    style: In-place modification of weights assumes NumPy array-like behavior - may fail if weights is a different type. What data types are expected for the weights parameter?

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  19. nvflare/fox/api/gcc.py, line 52-58 (link)

    logic: Race condition: num_whole_items_received is checked and modified without synchronization. Multiple threads could pass the limit check simultaneously, allowing more items than the limit and corrupting the count.

  20. nvflare/fox/examples/np/algos/strategies/avg_h.py, line 52 (link)

    style: Initialize total as 0.0 for consistency with _do_eval method

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  21. nvflare/fox/examples/np/algos/strategies/avg_seq.py, line 65 (link)

    logic: Client call uses optional=True which allows the algorithm to continue even if some clients fail, but the aggregation logic doesn't account for missing results

    Should the aggregation logic handle cases where some clients don't return results due to the optional=True parameter?

96 files reviewed, 21 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. nvflare/fox/api/gcc.py, line 116-124 (link)

    logic: race condition between lock release and abort check - abort_signal.triggered check outside lock could miss abort that occurs after lock release but before check

96 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (6)

  1. nvflare/fox/api/ctx.py, line 85-88 (link)

    logic: Context cleanup may not work correctly for nested contexts if exception occurs before __exit__ is called. The parent context restoration relies on __exit__ being invoked, but if an exception propagates without proper handling, the context chain could be broken.

  2. nvflare/fox/api/gcc.py, line 55-63 (link)

    logic: Race condition: num_whole_items_received check on line 56 and increment on line 62 are not atomic. If multiple threads call append() simultaneously (despite the note claiming thread safety via ResultWaiter), items could be appended beyond the limit.

    The update_lock only protects the body but threads could pass the check on line 56 before any increment happens.

  3. nvflare/fox/sys/utils.py, line 124-126 (link)

    style: Exception details are lost - only the exception type is logged. This makes debugging difficult when remote method calls fail.

  4. nvflare/fox/api/app.py, line 317-325 (link)

    style: Silent failures in initialization functions could lead to undefined behavior. If an init function raises an exception, it's caught by the caller but the app continues to be used.

    Consider logging or re-raising critical initialization failures.

  5. nvflare/fox/sys/controller.py, line 261 (link)

    style: Thread executor shutdown may not wait long enough if there are long-running tasks. Consider adding a timeout or checking task completion status before shutdown.

  6. nvflare/fox/sys/executor.py, line 100 (link)

    style: Thread executor shutdown may not wait long enough for in-flight tasks. Consider logging the number of cancelled tasks or adding timeout handling.

96 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (4)

  1. nvflare/fox/api/app.py, line 244-249 (link)

    style: regex pattern only allows letters, numbers, and underscores, but doesn't validate reserved Python keywords like class, def, return, etc. could conflict

  2. nvflare/fox/sim/simulator.py, line 79-86 (link)

    style: deepcopy fallback is good, but error message could guide users better by explaining when to use make_client_app (when objects contain unpicklable resources like locks, file handles, network connections)

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  3. nvflare/fox/sys/controller.py, line 260-261 (link)

    style: consider whether cancel_futures=True is appropriate - it may terminate in-flight federated operations abruptly

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  4. nvflare/fox/sys/backend.py, line 124 (link)

    style: consider checking if thread pool executor is shutting down before submitting group calls

96 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

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.

1 participant