Conversation
| from psqlgraph import PsqlGraphDriver | ||
|
|
||
|
|
||
| def shouldnt_delete(node): |
There was a problem hiding this comment.
Can you import this from esbuild/gdc_elasticsearch.py?
There was a problem hiding this comment.
I can move the shouldnt_delete method over there. I can also make it a single underscore "private" method in GDCDtataRelease since it is only used in GDCDataRelease().save_to_elasticsearch()
| if not args.test: | ||
| s3_md5 = g.save_to_s3(args, full_path_to_archive) | ||
| else: | ||
| from moto import mock_s3 |
There was a problem hiding this comment.
Testing/mocking functionality probably shouldn't be tied into the program. This should be done inside tests.
There was a problem hiding this comment.
That's fair. I mostly did this to try and avoid changing the tests as much as possible. The only change I had to do was add a flag to the test_build_scripts.py
https://github.com/NCI-GDC/esbuild/blob/dat-171/tests/test_build_scripts.py#L17-L28
| if args.test: | ||
| os.environ['SAVE_DIR'] = 'temp_dir' | ||
|
|
||
| os.environ['PG_HOST'] = 'localhost' |
There was a problem hiding this comment.
Stylistic comment, but typically we have functionality to pass vars as arguments, too. Something like:
parser.add_argument('-h', '--host', type=str,
help='host of the postgres server',
default=os.environ.get('PG_HOST', 'localhost'))
There was a problem hiding this comment.
I used environment variables to stay uniform in decisions made. That's basically the way all the code for elasticsearch was handled, so I decided to have everything as environment variables. If people want one way over the other I can always change this.
It should be noted that the tungsten file for this is nicer when you can separate the pulling of pillars into separate export lines instead of calling the large line at the bottom with a hand full of command line args.
|
|
||
| # compute md5sum of archive on disk | ||
| with open(full_path_to_archive, 'r') as f: | ||
| disk_archive_contents = f.read() |
There was a problem hiding this comment.
You have a large file here, so you could consider streaming it through instead.
See https://docs.python.org/2.7/library/hashlib.html
There was a problem hiding this comment.
That was one of my concerns, too. I wasn't sure how large an archived file would be, but I wanted to see how running it in a dev environment would turn out. There's code that uses FileChunkIO so I'll have some reference if we decide to change this.
|
|
||
| log = get_logger("gdc_datarelease") | ||
|
|
||
| class GDCDataRelease(object): |
There was a problem hiding this comment.
Could this extend GDCElasticsearch?
There was a problem hiding this comment.
This class saves to disk, uploads to es, and s3. What would be the benefit of inheriting from GDCElasticsearch only?
|
Also I don't remember commiting that pdf, and I don't know that I made any changes to it. Does it get generated when you run the tests or something? |
|
Well that's pretty spiffy. Am I safe to keep it since I didn't change the graph at all? |
|
@NCI-GDC/ucdevs Please feel free to leave comments |
added disk read/write operations tests for gdc_diskIO.py generate new archive name whenever call write_archive() added case for if there's a time when I'll get an empty index started moving gdces into its own export fixed derived_files issue so that tests passed fixed tests for gdc_diskIO.py fully incorperated upload to s3 functionality fixed every test to reflect changes minor style tweaks add extra comments added integrity checks and moved ./bin/around added md5sum check for integrity on s3 storage removed graph builders -> combined into one script (gdc_datarelease.py) added extra logging/comments added new tests fixed old tests updated readme, delete unused file
| file_count = self.es.count(index=new_index, doc_type="file")["count"] | ||
| case_count = self.es.count(index=new_index, doc_type="case")["count"] | ||
| ann_count = self.es.count(index=new_index, doc_type="annotation")["count"] | ||
| file_count = self.es.count(index=new_index, doc_type="file")["count"] |
There was a problem hiding this comment.
This is is actually against pep8 recommendations
5dba448 to
cf0b4da
Compare
a lot more logging general cleanup updated requirements.txt to include latest and greatest gdc code
ProfOak
changed the title
3 participants
This PR is kind of large (sorry!)
What's new?
./bin/gdc_datarelease.pythat does most of the thingsSAVE_DIRneeds to be set before runningWhat's different?
S3_HOSTS3_BUCKETS3_SECRET_KEYS3_ACCESS_KEYgdc_datarelease.py--legacyflagI have not successfully tested these changes in an esbuild environment, but am opening up a pull request for discussion.