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

Set vmen correctly, fixes #66 #70

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Set vmen correctly, fixes #66 #70

wants to merge 4 commits into from

Conversation

jebuss
Copy link
Member

@jebuss jebuss commented May 23, 2018

WIP

@jebuss
Copy link
Member Author

jebuss commented May 23, 2018

fixes #66

@jebuss jebuss requested a review from tarrox May 23, 2018 11:04
stream_runner = stream_runner_local
else:
run = [jar, xml, df]
run = [jar, xml, df, None, '{mb}'.format(0.9*vmem)]
Copy link
Member

Choose a reason for hiding this comment

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

This will raise a KeyError, I think you ment '{}mb'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think it should be '{}m' as this will work, I don't know if java works with 'mb', it should be checked while fixing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

shit I though I had them all. Missed that one. Good catch. mb works. worked before

@@ -21,7 +21,7 @@ def make_jobs(jar, xml, aux_source_path, output_directory, df_mapping, engine,
for num, indices in enumerate(split_indices):
df = df_mapping[indices.min(): indices.max()]

job = Job(stream_runner.run, [jar, xml, df, aux_source_path],
job = Job(stream_runner.run, [jar, xml, df, aux_source_path, '{}mb'.format(0.9*vmem)],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can result in bad errors, if vmem is lower then 10GB (the memory of everything else is set to around 1 GB). I think going for vmem-1000 and making sure that vmem can't be set lower then 2 GB for example, should be good. To create a check for vmem change its click type to: type=click.IntRange(2000, 1000000) (A maximum of 1 TB should be good enough for a little while).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand what you mean. This way the heap_size is always set to 90% vmem. so in case of 10GB vmem to 9GB heap_size and 1GB vmem to 900MB heap_size. Why should vmem-1000 help?

stream_runner = stream_runner_local
else:
run = [jar, xml, df]
run = [jar, xml, df, None, '{mb}'.format(0.9*vmem)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think it should be '{}m' as this will work, I don't know if java works with 'mb', it should be checked while fixing it.

@maxnoe maxnoe changed the title Fix#66 Set vmen correctly, fixes #66 Feb 12, 2019
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.

3 participants