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

DM-31043: Update ctrl_bps setup.cfg with flake8's max-doc-length=79 #37

Merged
merged 2 commits into from Jul 14, 2021

Conversation

mxk62
Copy link
Contributor

@mxk62 mxk62 commented Jul 13, 2021

No description provided.

Copy link
Collaborator

@MichelleGower MichelleGower 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 cleaning up a bunch of other stuff along the way. It looks like some spots were missed. I didn't comment on many of them in the code diff. In particular, check the tests and the import lines (in particular the from X import Y ones) to finish making all of them relative. Here are some find+grep commands I was doing to help check:
find . -type f ( -iname ".py" ! -name "init.py" ) -exec grep -n -H "^from lsst.ctrl.bps" {} ;
find . -type f ( -iname "
.py" ! -name "init.py" ) -exec grep -n -H " of " {} ; | grep " : "
find . -type f ( -iname ".py" ! -name "init.py" ) -exec grep -n -H -E "( .bps_|bps.bps_)" {} ;
find . -type f ( -iname "
.py" ! -name "init.py" ) -exec grep -n -H -E "lsst.ctrl.[^b]" {} ;
find . -type f ( -iname "*.py" ! -name "init.py" ) -exec grep -n -H -E "lsst.ctrl.bps.wms_service" {} ;

Do one more pass with the find+grep and one more round of building docs and pipelines_check run and then please merge.


Parameters
----------
config : `.bps_config.BpsConfig`
config : `lsst.ctrl.BpsConfig`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing ".bps"? (if not then all the others shouldn't have .bps in them either.)

@@ -35,22 +37,23 @@ def single_quantum_clustering(config, qgraph, name):

Parameters
----------
config : `~lsst.ctrl.bps.bps_config.BpsConfig`
config : `lsst.ctrl.bps.bps_config.BpsConfig`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need "bps_config"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah. Will fix.

@@ -225,7 +240,7 @@ def split_dependencies_by_tasks(self, dependencies):
return dependencies_by_tasks

def get_input_file(self, job_name):
""" Extracts the quantum graph file needed for a job
"""Extract the quantum graph file needed for a job
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed adding a period?

# Use dictionary plus template format string to create name.
# To avoid key errors from generic patterns, use defaultdict
# Use dictionary plus template format string to create name. To avoid
# key errors from generic patterns, use defaultdict
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing period

Some comments and docstings were exceeding maximal allowed length. Also,
some type specifications for function/methods were using now deprecated
(?) conventions.  I reformatted these comments and docstrings to make
them satisfy the LSST requirements.
The default search order for BpsConfig was defined in submit.py, a
module implementing function responsible for run submissions.  Moved the
definition to bps_config.py as it seems like much better choice.
@mxk62 mxk62 merged commit c386b2b into master Jul 14, 2021
@mxk62 mxk62 deleted the tickets/DM-31043 branch July 14, 2021 16:07
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.

None yet

2 participants