Skip to content
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.

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

Remove unnecessary contiguous() calls #1786

Open
22 tasks
ClaudiaComito opened this issue Feb 10, 2025 · 0 comments
Open
22 tasks

Remove unnecessary contiguous() calls #1786

ClaudiaComito opened this issue Feb 10, 2025 · 0 comments
Labels
enhancement New feature or request
Milestone

Comments

@ClaudiaComito
Copy link
Contributor

ClaudiaComito commented Feb 10, 2025

Related
#1765

Remove unnecessary contiguous() calls (Post-Optimization)

Information: grep -ir 'contiguous(' *
Text & annoyingness: Gemini 2.0 Experimental Advanced

Context: Recent changes #1765 to the codebase have potentially eliminated the need for some previously enforced contiguity constraints. This issue tracks the removal of unnecessary .contiguous() calls.

General Strategy:

  1. Identify Removable Calls: Based on the recent codebase modifications, identify .contiguous() calls that are no longer strictly required. The grep results below provide a starting point.
  2. Verify Non-Necessity: For each identified call, double-check that removing it will not break any functionality or introduce subtle errors. This involves understanding the downstream operations and their memory layout requirements. Tests should now pass without these contiguous() calls.
  3. Remove and Test: Remove the contiguous() call and run comprehensive tests (unit tests, integration tests, and performance benchmarks).
  4. Performance Benchmarking: Crucially, compare performance before and after the removal. The goal is to improve performance by avoiding unnecessary data copies. If performance degrades, re-evaluate the removal.
  5. Consider Views: In some instances, where data reshaping is involved, determine if a view of the original data is sufficient instead of creating a contiguous copy.

Specific Locations (from grep results):

  • core/manipulations.py:

    • Remove torch.diagonal(...).contiguous(). Confirm that subsequent operations can handle non-contiguous diagonals.
    • Remove _[local_slice].contiguous(). Verify that the code using this slice works correctly with non-contiguous data.
  • core/signal.py:

    • Remove signal_filtered.contiguous(). Ensure that the filtering operation and subsequent processing steps no longer require contiguity.
  • core/linalg/qr.py:

    • Remove R_loc = R_loc.contiguous() (both instances). The original comment ("required for all the communication ops lateron") should now be invalid. Verify that communication works correctly without forcing contiguity here.
    • Remove Q_buf = Q_buf.contiguous(). Similar to R_loc, confirm that communication and other operations no longer require Q_buf to be contiguous at this point.
  • core/memory.py:

    • Remove x.permute(dims).contiguous(). Validate that the permuted tensor's usage doesn't rely on contiguity.
  • core/dndarray.py:

    • Remove self.__array[ix].clone().contiguous(). Confirm that the cloned slice's use case no longer necessitates contiguity.
  • core/random.py:

    • Remove return x_0.contiguous(), x_1.contiguous(), lshape, lslice. Verify that the returned tensors are used in a way that doesn't require them to be contiguous.
  • core/communication.py:

    • Review all instances of .contiguous() and is_contiguous(). The goal is to remove as many of these as possible, now that the underlying requirement for contiguity has been relaxed.
      • mpi_type.Create_contiguous(left_over).Commit(): This is likely still necessary for MPI datatype creation. Leave this unchanged unless the MPI usage is also being modified.
      • Conditional calls: if not sendbuf.is_contiguous(): tmp = sendbuf.contiguous(): These were previously necessary. Remove these conditions and the tmp = ... lines. The assumption is that non-contiguous sends/receives are now handled correctly.
  • core/factories.py:

    • Remove _ = obj[slices].contiguous(). Ensure that the result of this slicing operation is not required to be contiguous.
  • core/tests/test_communication.py: This file contains a lot of tests to check is a tensor is contiguous.

    • Adapt/remove tests. After removing contiguous() calls, some tests might now fail (because data is no longer contiguous where it used to be). Adapt the assertions or the entire tests based on the changes and the new expected behavior.
@ClaudiaComito ClaudiaComito added the enhancement New feature or request label Feb 10, 2025
@ClaudiaComito ClaudiaComito added this to the 1.6 milestone Feb 10, 2025
@github-project-automation github-project-automation bot moved this to Todo in Roadmap Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

1 participant