Skip to content

Refactor#2

Open
cpankow wants to merge 3 commits into
reedessick:masterfrom
cpankow:refactor
Open

Refactor#2
cpankow wants to merge 3 commits into
reedessick:masterfrom
cpankow:refactor

Conversation

@cpankow

@cpankow cpankow commented Dec 8, 2016

Copy link
Copy Markdown

I've refactored the ligolw_segment_query_segdb call to use the python API. I've also written out the call to ligolw_segment_query_segdb --dmt to use a simplified file location and retrieval setup. A brief test shows that both work for a test gracedb event. Please review for clarity, efficiency, and any subtler bugs. I have not tested that the logging to gracedb is identical to the previous incarnation.

@reedessick

Copy link
Copy Markdown
Owner

Can you elaborate on exactly what testing you've done? You must have supplied the --skip-gracedb-upload option, correct? For instance, your call to writeMessage on

https://github.com/cpankow/SegDB2GraceDB/blob/refactor/bin/seglogic.py#L335

is not defined. Instead, writeLogMessage is defined, but does not actually return anything. I hesitate to merge anything into the production repo with incomplete development like this.

Also, can you please specify which GraceId you used for testing?

@cpankow

cpankow commented Dec 8, 2016

Copy link
Copy Markdown
Author

I modified the ini file to not point to directories I did not have access to, then this command was run:
python seglogic.py -n -g G264894 -v ../etc/seglogic.ini

Twice. Once unmodified and once with the segdb route manually forced. Your note on the return value is absolutely correct (there will be another commit coming to fix that). I should note that this PR will be updated as time goes on to address issues, and when you feel comfortable merging, then we can merge. I wanted substantive feedback on the way the change was happening currently.

I'm happy to do more thorough tests on a live gracedb instance, I just need a good test event to do so.

@reedessick

Copy link
Copy Markdown
Owner

You can (and should) test to your heart's desire using gracedb-test. For that, you'll need to modify the "gracedb_url" option in the config file to point to https://gracedb-test.ligo.org/api/ and pick any GraceId from there. Note, the dmt files are cleaned up periodically at CIT so you can't use an ancient event.

Hopefully I'll be able to test this a bit myself in the next few weeks and give you more concrete development suggestions.

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.

2 participants