Fix weird things with space checks in some files (see #1167 #1166)

[SensibleSalmon]

See #1166, #1167

Blocked by: #1169

Lightweight argparse cleanup, making space checks use the right directory instead of cwd.

[ctb]

@bocajnotnef do you see anything in the code that would lead to this problem?

http://lists.idyll.org/pipermail/khmer/2015-July/000736.html

I'm stumped...

[SensibleSalmon]

I recall one of the checks was only checking the directory the script was
called from, not the directory of the given output filename. I forget which
issue specifically but it's one of the ones involved in this pull.

On Tue, Jul 14, 2015, 09:53 C. Titus Brown notifications@github.com wrote:

@bocajnotnef https://github.com/bocajnotnef do you see anything in the
code that would lead to this problem?

http://lists.idyll.org/pipermail/khmer/2015-July/000736.html

I'm stumped...

—
Reply to this email directly or view it on GitHub
#1170 (comment).

[SensibleSalmon]

I'll dig up which issue in a minute.

On Tue, Jul 14, 2015, 10:23 Jake Fenton bocajnotnef@gmail.com wrote:

I recall one of the checks was only checking the directory the script was
called from, not the directory of the given output filename. I forget which
issue specifically but it's one of the ones involved in this pull.

On Tue, Jul 14, 2015, 09:53 C. Titus Brown notifications@github.com
wrote:

@bocajnotnef https://github.com/bocajnotnef do you see anything in the
code that would lead to this problem?

http://lists.idyll.org/pipermail/khmer/2015-July/000736.html

I'm stumped...

—
Reply to this email directly or view it on GitHub
#1170 (comment).

[ctb]

OK, could you track it down, please? thanks!

[SensibleSalmon]

Issue #1166, line 120ish in kfile.py (https://github.com/dib-lab/khmer/pull/1170/files#diff-46d242f8c7f4ca46fd5c784bd24184c7R120)

 """Check we have enough size to write a hash table."""                      
    hash_size = khmer_args._calculate_tablesize(args, hashtype)                 

    cwd = os.getcwd()                                                           
    dir_path = os.path.dirname(os.path.realpath(cwd))                           
    target = os.statvfs(dir_path)                             

We grab cwd and not os.path.dirname(args.out) or summuch.

[SensibleSalmon]

in this case a --force should solve the issue until a fix gets implemented.

[ctb]

By my reading, that's not the problem in the email, tho?

Titus Brown, ctbrown@ucdavis.edu

On Jul 14, 2015, at 10:32 AM, Jake Fenton notifications@github.com wrote:

Issue #1166, line 120ish in kfile.py (https://github.com/dib-lab/khmer/pull/1170/files#diff-46d242f8c7f4ca46fd5c784bd24184c7R120)

"""Check we have enough size to write a hash table."""
hash_size = khmer_args._calculate_tablesize(args, hashtype)

cwd = os.getcwd()                                                           
dir_path = os.path.dirname(os.path.realpath(cwd))                           
target = os.statvfs(dir_path)                             

We grab cwd and not os.path.dirname(args.out) or summuch.

—
Reply to this email directly or view it on GitHub.

[SensibleSalmon]

Looks to be; error is: ERROR: Not enough free space on disk for saved table files; (from the email).

Output from kfile on line ~130 (https://github.com/dib-lab/khmer/blob/master/khmer/kfile.py#L130):

    if size_diff > 0:
        print("ERROR: Not enough free space on disk "
              "for saved table files;"
              "       Need at least %s GB more."
              % (float(size_diff) / 1e9,), file=sys.stderr)

[SensibleSalmon]

Ahh, I see--He is calling norm-by-med from his mnt directory. I'd missed that.

[ctb]

Yep. I wonder if os.stat on a mount point returns parent fs info?

Titus Brown, ctbrown@ucdavis.edu

On Jul 14, 2015, at 10:46 AM, Jake Fenton notifications@github.com wrote:

Ahh, I see--He is calling norm-by-med from his mnt directory. I'd missed that.

—
Reply to this email directly or view it on GitHub.

[SensibleSalmon]

It's behaving as if what I said is the problem but since he's calling norm-by-med on the mounted partition this shouldn't be happening. The only explanation I can think of is if he installed khmer to the system (or a virtualenv) and os.getcwd() is somehow getting the directory of where khmer is installed and not where it's being called from--which seems extremely weird.

I'm ruling out that it's something to do with os.statvfs since it's correctly reporting the amount of free space on the system partition.

Could it be that os.path.dirname(os.path.realpath(cwd)) is getting root as the parent FS and is checking there...?

[SensibleSalmon]

Did some quick testing in iPython--This doesn't appear to be it.

Ran the kfile space check on a file in my homedir, got:

In [5]: cwd = os.getcwd()

In [6]: cwd
Out[6]: '/home/bocajnotnef'

In [8]: res = os.statvfs(os.path.realpath(cwd))

In [9]: fs = res.f_frsize * res.f_bavail

In [10]: fs
Out[10]: 75171151872

Running the same on a file on an SD card:

In [18]: dir = os.path.dirname(os.path.realpath('/media/bocajnotnef/Mercury/test.file'))

In [19]: dir
Out[19]: '/media/bocajnotnef/Mercury'

In [20]: res = os.statvfs(dir)

In [21]: fs = res.f_frsize * res.f_bavail

In [22]: fs
Out[22]: 67091300352

I have ~70gigs free on my system partition and the SD card is 64 gigs, so those numbers look about right.

[ctb]

On Tue, Jul 14, 2015 at 08:00:44AM -0700, Jake Fenton wrote:

Did some quick testing in iPython--This doesn't appear to be it.

I concur.

Weird, well, I will let them know.

--titus

[SensibleSalmon]

@ctb Updated; CR/merge?

[ctb]

Ref #1169 (comment), what do you think about breaking it out like this?

tablesize = khmer_args.calculate_tablesize(args)
check_space_for_hashtable(tablesize)

in the various scripts?

I definitely foresee a future where tablesize is determined from more args, but the check space function doesn't need to know that - it just needs to know the estimated size.

[SensibleSalmon]

+1 for that solution.

[SensibleSalmon]

@mr-c @ctb CR, please?

I feel I should add a test to avoid the previous behaviour (Testing the wrong directory's space) but I'm unsure of how to come up with a test scenario for this--we can't assume that whatever system we're testing on has a convienently mounted drive for us to use.

[ctb]

Ref #618.

[ctb]

Also, can you take a look at #668?

[SensibleSalmon]

I'd need access to the HPCC (or a similar setup) to verify the behaviour
still exists (or have @mr-c confirm)

On Sat, Jul 18, 2015, 13:04 C. Titus Brown notifications@github.com wrote:

Also, can you take a look at #668
#668?

—
Reply to this email directly or view it on GitHub
#1170 (comment).

[ctb]

On Sat, Jul 18, 2015 at 01:07:06PM -0700, Jake Fenton wrote:

I'd need access to the HPCC (or a similar setup) to verify the behaviour
still exists (or have @mr-c confirm)

You might first check to see what the behavior is for disks to which
you have read-only access.

[SensibleSalmon]

Seems to be what we'd expect--that is, it yells at you that you don't have permissions. (comments in #668)

[ctb]

Yes, but it doesn't cause the same error that @mr-c saw. So it looks like there's some oddity on HPC systems (?).

[ctb]

I think 'calculate_tablesize' should just take 'args', so as to accomodate things like -U/--unique-kmers. Otherwise LGTM on a first pass!

Note that you could fix #1179 rather casually at this point, too.

[SensibleSalmon]

What's strange is khmer thinks there was no free space at all--that doesn't
look like it's reading the wrong directory.

I'm wondering if not having read permissions would show up as having no
free space in a stat.

Though, there isn't anything about not having read perms in the issue.

On Sun, Jul 19, 2015, 08:10 C. Titus Brown notifications@github.com wrote:

Yes, but it doesn't cause the same error that @mr-c
https://github.com/mr-c saw. So it looks like there's some oddity on
HPC systems (?).

—
Reply to this email directly or view it on GitHub
#1170 (comment).

[ctb]

On Sun, Jul 19, 2015 at 08:15:28AM -0700, Jake Fenton wrote:

What's strange is khmer thinks there was no free space at all--that doesn't
look like it's reading the wrong directory.

I'm wondering if not having read permissions would show up as having no
free space in a stat.

Maybe? 'df' shows file size for things you don't have read perms to,
I think. But it's a good point and easy enough to test.

Though, there isn't anything about not having read perms in the issue.

True.

[SensibleSalmon]

I'll do some testing in a bit, as well as integrating -U

On Sun, Jul 19, 2015, 08:19 C. Titus Brown notifications@github.com wrote:

On Sun, Jul 19, 2015 at 08:15:28AM -0700, Jake Fenton wrote:

What's strange is khmer thinks there was no free space at all--that
doesn't
look like it's reading the wrong directory.

I'm wondering if not having read permissions would show up as having no
free space in a stat.

Maybe? 'df' shows file size for things you don't have read perms to,
I think. But it's a good point and easy enough to test.

Though, there isn't anything about not having read perms in the issue.

True.

—
Reply to this email directly or view it on GitHub
#1170 (comment).

[SensibleSalmon]

Well, that wasn't it.

ERROR: File /tmp/testingdir/table does not have write permission; exiting
(env)1 bocajnotnef@GCU-Caconym:~/git/khmer⟫ ls /tmp/testingdir/
ls: cannot open directory /tmp/testingdir/: Permission denied
(env)2 bocajnotnef@GCU-Caconym:~/git/khmer⟫ 

it is also possible to stat the directory:

In [9]: dir_path = os.path.dirname(os.path.realpath('/tmp/testingdir/table'))

In [10]: target = os.statvfs(dir_path)

In [11]: free_space = target.f_frsize * target.f_bavail

In [12]: free_space
Out[12]: 75911532544

[ctb]

hey @bocajnotnef, can you also fix the %s to be a %.1f in check_space_for_hashtable, around line 135 of khmer/kfile.py?

Need at least %s GB more.

and then check this box:

• fixed GB output to truncate float

[SensibleSalmon]

figured out weirdness with HPCC stuff--this should fix it.

@mr-c @ctb @camillescott CR/Merge, please?

[ctb]

?

[SensibleSalmon]

@ctb Updated

[ctb]

Cleaned up minor slop in 4ecd60b; merging.

[ctb]

BTW, @bocajnotnef, don't put the issue numbers in the pull request subject; you can't click on 'em. Better to just put them in the description somewhere, so as to link issues & PR.

[SensibleSalmon]

Ahh, alright. Wilco.

On Tue, Jul 21, 2015 at 6:57 PM C. Titus Brown notifications@github.com
wrote:

BTW, @bocajnotnef https://github.com/bocajnotnef, don't put the issue
numbers in the pull request subject; you can't click on 'em. Better to just
put them in the description somewhere, so as to link issues & PR.

—
Reply to this email directly or view it on GitHub
#1170 (comment).