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 indexing methods from public API #588

Closed
5 tasks done
bdice opened this issue Aug 2, 2021 · 8 comments
Closed
5 tasks done

Remove indexing methods from public API #588

bdice opened this issue Aug 2, 2021 · 8 comments
Milestone

Comments

@bdice
Copy link
Member

bdice commented Aug 2, 2021

Feature description

In PR #580, I remove a large portion of the indexing.py module. Below are some follow-up tasks to be done after #580 is merged.

  • Investigate making Project.index into a private method. This may eliminate the need for most/all of the contents of indexing.py. (See continued discussion below.)
  • Investigate removing index argument from a number of Project methods. (See continued discussion below.)
  • Review package documentation and remove indexing topics.
  • Review signac-docs to remove indexing topics. see Remove docs about indexing signac-docs#152
  • Review signac-examples to remove indexing topics.
@bdice bdice added this to the v2.0.0 milestone Aug 2, 2021
@bdice
Copy link
Member Author

bdice commented Aug 2, 2021

Discussed in developers meeting. @atravitz and I will work through this checklist and coordinate via Slack.

We agreed that Project.index can be deprecated in 1.x and removed in 2.0, which will eliminate the need for most/all of indexing.py.

Next steps (all of these can be done in parallel):

  • Wait for Remove public API of indexing module. #580 to be reviewed/merged.
  • In 1.x / master:
    • Deprecate Project.index in 1.x.
    • Add deprecation warnings for all Project methods with non-empty index arguments in 1.x.
  • In 2.0 / next:
    • Make Project.index a private method, remove all unused arguments (only include_job_document should remain, based on current usage).
    • Remove index arguments from all Project methods.

@bdice
Copy link
Member Author

bdice commented Aug 9, 2021

Next steps:

  • Check whether the Project's _sp_index() method and _index_cache attribute are redundant with _sp_cache attribute and _index method. There may also be some overlap with the _build_index or _update_in_memory_cache methods.
  • Do we need to support custom Job subclasses that do not adhere to the same workspace layout? This looks like it's incompletely supported in the Project class and should probably be removed.
    wd = self.workspace() if self.Job is Job else None

@atravitz
Copy link
Collaborator

The indexing example in signac-examples is already deprecated. Should I remove it now, or wait for the 2.0 release?

@bdice
Copy link
Member Author

bdice commented Aug 11, 2021

@atravitz No need to wait, we can go ahead and modernize and clean up examples!

@stale
Copy link

stale bot commented Jan 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 3, 2022
@bdice
Copy link
Member Author

bdice commented Jan 4, 2022

Update since this issue is marked as stale:

Next steps:

  • Check whether the Project's _sp_index() method and _index_cache attribute are redundant with _sp_cache attribute and _index method. There may also be some overlap with the _build_index or _update_in_memory_cache methods.

This is under active development in #652. I'll finalize that PR.

  • Do we need to support custom Job subclasses that do not adhere to the same workspace layout? This looks like it's incompletely supported in the Project class and should probably be removed.
    wd = self.workspace() if self.Job is Job else None

I think this still needs to be done. It should be a small PR.

@stale stale bot removed the stale label Jan 4, 2022
@vyasr
Copy link
Contributor

vyasr commented Feb 21, 2022

@bdice is that last task still to-do? Can we close this issue once that's been addressed?

@bdice
Copy link
Member Author

bdice commented Feb 27, 2022

Yes, that is the last task. I opened #693 to resolve this.

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

No branches or pull requests

3 participants