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

Galactic Radio Telescope Update #4376

Merged
merged 34 commits into from Aug 7, 2017

Conversation

Projects
None yet
3 participants
@erasche
Copy link
Member

commented Aug 4, 2017

replaces #2768. Many changes and optimisations:

  • query tables directly with sqlalchemy rather than allowing it to decode into objects
  • optional sanitisation (based on original work by @ericenns) at the tool level and at the parameter level (slow).
  • separate reporting + uploading steps to allow admins behind firewalls to help out with the GRT projcet.
  • remove instance metadata nonsense since other projects want to handle that and I don't care about it. I just want your jobs :)
  • export performance is on the order of 150k jobs / minute (for admin reference)

Updated backend is available, but it is quite boring. Just accepts reports, and then processes them offline (on cron), and generates some reports.

erasche added some commits Jul 21, 2017

erasche added some commits Aug 4, 2017

@erasche erasche requested a review from bgruening Aug 7, 2017

@bgruening
Copy link
Member

left a comment

Thanks @erasche! Some small comments inline.

@@ -0,0 +1,69 @@
#!/usr/bin/env python

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

We should but this into it's own subdir.

This comment has been minimized.

Copy link
@erasche

erasche Aug 7, 2017

Author Member

sure, I can do this. I guess scripts is getting a bit full.

help="Set the logging level", default='warning')
args = parser.parse_args()

logging.info('Loading GRT ini...')

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

ini? config?

with open(args.config) as handle:
config = yaml.load(handle)
except Exception:
logging.info('Using default GRT Configuration')

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

s/C/c/

local_reports = [x.strip('.json') for x in os.listdir(REPORT_DIR) if x.endswith('.json')]
for report_id in local_reports:
if report_id not in remote_reports:
print("Uploading %s" % report_id)

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

logging.info

'identifier': report_id
}
r = requests.post(GRT_URL + 'api/v2/upload', files=files, headers=headers, data=data)
print(r.json())

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

This line should be removed, I guess.


print('Loading Galaxy...')
model, object_store, engine = _init(config_dict['galaxy_config'])
logging.info('Using default GRT Configuration')

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

s/Configuration/configuration/

import yaml
import re
import logging
# logging.getLogger('sqlalchemy.engine').setLevel(logging.INFO)

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

This can be removed.

.filter(model.Job.id <= min(end_job_id, offset_start + args.batch_size)) \
.all():

# TODO: blacklisted

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 7, 2017

Member

Can you write a little bit more about the TODO here.

erasche added some commits Aug 7, 2017

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

Addressed.

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

There is currently some small problem with "share_toolbox" so I've defaulted it to false. Having the full toolbox is less important but still a useful thing to collect since we can produce useful reports for admins. I will patch this in a future PR, but it is not a blocking issue for this one I believe.

@jmchilton jmchilton merged commit 68c7b09 into galaxyproject:dev Aug 7, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

Thanks for the update and continued work on this @erasche - I look forward to great things!

One thing I'd like to see is that if we are claiming to block certain parameters that we provide a test case to verify this blocking is in fact happening and doesn't get broken at some point. Especially given the complexity of nested parameters and such. Just something to think about for future development.

Thanks again though!

@erasche

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2017

@jmchilton where would be a good place to do that? Should I move sanitization into lib/galaxy/ somewhere and then provide a test case?

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

I think you should be able to setup an API test that runs a bunch of jobs and then calls the script to generate the data dump directly from the API test - use the path to the test case Python file to find galaxy_root and then append the scripts directory - call Python directly against that script after setting up test files to target the test setup. I'm not sure things need to be refactored. It might be more complicated than that - I don't know - but it seems straight forward at first glance. Integration tests allow more customization of Galaxy's config prior to starting up Galaxy - so if it is more complicated than an API test allows and you needed to target that directly you could think about doing it there.

@erasche erasche deleted the erasche:grt-update-2 branch Aug 21, 2017

@martenson martenson referenced this pull request Apr 18, 2018

Open

food for thought #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.