Skip to content

Disable Block Allocation by default #473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 2, 2024
Merged

Disable Block Allocation by default #473

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions executorlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def __init__(
flux_executor_nesting: bool = False,
pysqa_config_directory: Optional[str] = None,
hostname_localhost: Optional[bool] = None,
block_allocation: bool = True,
block_allocation: bool = False,
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the gathered information, I can now provide the final response:

Document the default value change and add migration guidance

The change to disable block allocation by default requires documentation updates and migration guidance because:

  1. The documentation in docs/installation.md already mentions implications of block_allocation=False in HPC context, indicating it's a significant setting
  2. Several tests and examples use Executor() without explicitly setting block_allocation, which will be affected by the default change
  3. No CHANGELOG or migration guide exists to document this behavioral change

Recommendations:

  • Add a CHANGELOG.md file documenting this breaking change
  • Update docstrings and documentation to reflect the new default value
  • Consider a deprecation cycle as suggested in the original review
  • Add migration guidance for users who rely on the old behavior
🔗 Analysis chain

Verify impact of disabling block allocation by default.

While this change aligns with the PR objective, changing default behavior could impact existing code that relies on the implicit block allocation being enabled. Consider:

  1. Adding a deprecation warning when block allocation is enabled to help users transition
  2. Documenting this change in CHANGELOG.md
  3. Updating any examples or documentation that might assume the old default

Let's check for potential impacts:

Consider adding a deprecation cycle:

  1. First deprecate the True default with a warning
  2. Then change the default to False in a future major version
    This would give users time to update their code explicitly.

Also applies to: 117-117

Let me analyze the results and generate additional verification scripts to check the documentation and potential impacts.

The first script will help us understand how block_allocation is documented and if the change in default value is properly documented. The second script will identify any examples or tests that might be relying on the implicit default value. The third script will check if there are any migration guides or changelog entries for this change.

This additional context will help us make a more informed decision about the potential impact and necessary documentation updates.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct Executor instantiations that might be affected
rg "Executor\(" --type py -B 2 -A 2

# Look for documentation files that might need updates
fd -e md -e rst | xargs rg -l "block_allocation"

# Check for examples that might need updates
fd "example" --type py | xargs rg -l "Executor"

Length of output: 29896

Script:

#!/bin/bash
# Check documentation for block_allocation mentions and its default value
rg -A 5 -B 5 "block_allocation" docs/

# Check if there are any examples or tests that rely on the default behavior
rg "Executor\([^)]*\)" --type py | rg -v "block_allocation"

# Look for any migration guides or changelog entries
fd "CHANGELOG|MIGRATION" -i

Length of output: 2390

init_function: Optional[callable] = None,
disable_dependencies: bool = False,
refresh_rate: float = 0.01,
Expand All @@ -114,7 +114,7 @@ def __new__(
flux_executor_nesting: bool = False,
pysqa_config_directory: Optional[str] = None,
hostname_localhost: Optional[bool] = None,
block_allocation: bool = True,
block_allocation: bool = False,
init_function: Optional[callable] = None,
disable_dependencies: bool = False,
refresh_rate: float = 0.01,
Expand Down
Loading