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.

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

Jump to bottom

Remove doc-filter after query switched to unified syntax #738

Merged
merged 4 commits into from
Apr 14, 2023
Merged

Remove doc-filter after query switched to unified syntax #738

merged 4 commits into from
Apr 14, 2023

Conversation

cbkerr
Copy link
Member

@cbkerr cbkerr commented Apr 14, 2023 •
edited
Loading

Description

Motivation and Context

Resolves #737

See also glotzerlab/signac#516

However, this conflicts with something in the changelog:
"Deprecated doc_filter arguments, which are replaced by namespaced filters. Due to their long history, doc_filter arguments will still be accepted in signac 2.0 and will only be removed in 3.0 (#516)."

Checklist:

@cbkerr cbkerr requested review from a team as code owners April 14, 2023 18:51
@cbkerr cbkerr requested review from bdice and syjlee and removed request for a team April 14, 2023 18:51
@cbkerr cbkerr requested review from tommy-waltmann and removed request for syjlee April 14, 2023 18:53
bdice
bdice approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good to me. The changelog you mentioned should be updated -- we removed doc filters in 2.0 and chose not to wait until 3.0. Can you file a PR for that? (edit: I did it here: glotzerlab/signac#916) Even though it is a changelog entry for a past version, its guidance is inaccurate, so I would think it should be rewritten.

I suppose this code path wasn't being run by tests. It would be good to add a test for filtering on the command line.

@codecov
Copy link

codecov bot commented Apr 14, 2023 •
edited
Loading

Codecov Report

Merging #738 (ad26207) into main (4590e42) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head ad26207 differs from pull request most recent head c45c6ed. Consider uploading reports for the commit c45c6ed to get more accurate results

@@           Coverage Diff           @@
##             main     #738   +/-   ##
=======================================
  Coverage   68.77%   68.78%           
=======================================
  Files          42       42           
  Lines        4196     4194    -2     
  Branches     1032     1032           
=======================================
- Hits         2886     2885    -1     
+ Misses       1099     1098    -1     
  Partials      211      211
Impacted Files Coverage Δ
flow/project.py 82.85% <0.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bdice
Copy link
Member

bdice commented Apr 14, 2023

I opened a PR to fix signac's changelog: glotzerlab/signac#916

@cbkerr
Copy link
Member Author

cbkerr commented Apr 14, 2023

@bdice I added a test and confirmed that it fails without the change introduced by this PR

tommy-waltmann
tommy-waltmann approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@cbkerr cbkerr merged commit 3b5a514 into main Apr 14, 2023
@cbkerr cbkerr deleted the fix/remove-doc-filter branch April 14, 2023 20:24
@tommy-waltmann tommy-waltmann mentioned this pull request Apr 26, 2023
Merged
3 tasks
b-butler pushed a commit that referenced this pull request Jul 27, 2023
@cbkerr @pre-commit-ci @b-butler
Remove doc-filter after query switched to unified syntax (#738)
c571532 
* First pass at fix

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add filter test

* Update changelog

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
b-butler added a commit that referenced this pull request Oct 23, 2023
@b-butler @pre-commit-ci
@cbkerr @bdice @rayasare @dependabot
Refactor submission templates (#722)
ca207a6 
* refactor: Add _get_scheduler_values method to environments

This moves much of the task and number of nodes logic to the
environments where it is easier to manage the more complicated logic.

* refactor: Port some templates over as examples of new style.

* feat: Create way for environment classes to use partition config

CPUS and GPUS per partition are in theory supported.

* refactor: Update all environment which I have access to.

* fix: Provide default node behavior for _get_scheduler_values

Use sentinal of -1 to denote no node structure and always return 1 node
requested for either CPU or GPU tasks.

* fix: Fix typo in setting resources to Jinja2 context

* fix: Delta template partition node submissions.

* refactor: Provide infrastrucure to correctly warn on low-resources

Add the ComputeEnvironment._shared_partitions attribute to check if less
than single node submissions should be allowed in
ComputeEnvironment._get_scheduler_values.

* refactor (WIP): Add _shared_partitions to Delta env for testing.

* fixup: removing operation from _get_scheduler_values

* fix: Delta template

The Delta template is now tested and works.

* fix: Bridges2 template and environment.

The environment is now tested.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* refactor: Reset Stampede2 template

I do not have access to the cluster, so to prevent regressions, I am
reseting this.

* fix: Update shared partitions specifications in environments.

* fix: Only specify node request on non-shared partitions (Expanse).

* [pre-commit.ci] pre-commit autoupdate (#736)

updates:
- [github.com/psf/black: 23.1.0 → 23.3.0](psf/black@23.1.0...23.3.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Remove doc-filter after query switched to unified syntax (#738)

* First pass at fix

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add filter test

* Update changelog

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* fix: Delta hostname regex. (#740)

Delta changed their compute node hostnames in their April 19th
maintanance. This fixes the detection of the delta environment.

* Release/0.25.1 (#742)

* doc: Update changelog.

* Bump up to version 0.25.1.

* Add Frontier Environment (#743)

* feat: Add the Frontier supercomputer to environments.

* test: Add Frontier to template testing.

* test: Update environment test template generation to signac 2.0

* doc: Update changelog

* doc: Add Frontier documentation.

* doc: Update incode comment clarity

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

---------

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

* Feature/469 check status for specific operations (#725)

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

* Build(deps-dev): Bump pre-commit from 3.2.1 to 3.2.2 (#746)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build(deps): Bump pytest from 7.2.2 to 7.3.1 (#744)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build(deps): Bump coverage from 7.2.2 to 7.2.5 (#745)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>

* Feat/template testing (#747)

* feat (WIP): create flow CLI subcommand for testing templates

* feat: Finish new CLI option.

* test: flow test-workflow.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* doc: Add test-workflow to documentation.

* doc: Add changes to changelog.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Build(deps): Bump ruamel-yaml from 0.17.21 to 0.17.31 (#752)

Bumps [ruamel-yaml](https://sourceforge.net/p/ruamel-yaml/code/ci/default/tree) from 0.17.21 to 0.17.31.

---
updated-dependencies:
- dependency-name: ruamel-yaml
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build(deps): Bump pytest-cov from 4.0.0 to 4.1.0 (#751)

Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 4.0.0 to 4.1.0.
- [Changelog](https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst)
- [Commits](pytest-dev/pytest-cov@v4.0.0...v4.1.0)

---
updated-dependencies:
- dependency-name: pytest-cov
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build(deps-dev): Bump pre-commit from 3.2.2 to 3.3.2 (#750)

Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 3.2.2 to 3.3.2.
- [Release notes](https://github.com/pre-commit/pre-commit/releases)
- [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md)
- [Commits](pre-commit/pre-commit@v3.2.2...v3.3.2)

---
updated-dependencies:
- dependency-name: pre-commit
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Build(deps): Bump coverage from 7.2.5 to 7.2.7 (#749)

Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.2.5 to 7.2.7.
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.2.5...7.2.7)

---
updated-dependencies:
- dependency-name: coverage
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: UMich Greatlakes environment configuration

* fix: Correctly GPU raise errors in Greatlakes template

Error when GPU partition is requested without GPUs and when GPUs are
requested without a GPU partition.

* refactor: Small changes to new template environment code

* refactor: Convert frontier template to new format

* refactor: Frontier remove FrontierEnvironment.calc_num_nodes

* feat: Finish conversion of Frontier environment.

* refactor: Remove unnecessary empty shared_partition sets

* fix: Frontier's allowable CPU use

* test: Update submission scripts

* fix: Stampede2Environment._get_scheduler_values

* fix: remove Stampede2Environment._get_scheduler_values

* test: Update template tests.

* fix: typo in andes

* test: Update tests to new code.

* style: Remove incorrect comments.

* Fix: Andes template typo

* test: Update Andes template

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Corwin Kerr <cbkerr@umich.edu>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: rayasare <43545382+rayasare@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@tommy-waltmann tommy-waltmann tommy-waltmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when running jobs using -f on command line
3 participants
@cbkerr @bdice @tommy-waltmann