Skip to content

Allow >1 CPU per task, and some misc clean up#45

Merged
phargogh merged 10 commits into
mainfrom
bugfix/33
Jun 4, 2026
Merged

Allow >1 CPU per task, and some misc clean up#45
phargogh merged 10 commits into
mainfrom
bugfix/33

Conversation

@emlys

@emlys emlys commented Jun 1, 2026

Copy link
Copy Markdown
Member

Fixes #33 by setting --cpus-per-task to 2.

Throwing in some other small clean-up items, and downgrading some resources to save on costs.

import requests


def download_and_extract_datastack(datastack_url, extracted_datastack_dir):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was not longer needed since we are downloading and extracting the datastack with curl and tar.

dynamic = ["version"] # the version is provided dynamically by setuptools_scm
dependencies = [
"flask",
"natcap.invest",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No longer a dependency since download_and_extract_datastack was removed

micromamba activate invest_env
pip install setuptools setuptools_scm build cython babel
pip install --no-build-isolation git+https://github.com/natcap/pygeoprocessing.git
pip install --no-build-isolation git+https://github.com/emlys/invest.git@bugfix/2572

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Install the not-yet-released changes that fixed the memory leak issue

project_id = var.project_id
region = var.region
startup_script = "startup_script.sh"
startup_script = "/home/bin/startup_script.sh"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to an absolute path to prevent issues depending where this is run from.

allow_automatic_updates = false
labels = var.labels
machine_type = "c2-standard-8"
machine_type = "c2-standard-4"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Downgraded for cost savings.

}
}

CPUS_PER_TASK = 2 # number of cpus per task to submit to sbatch

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

c2-standard-4 has 4 vCPUs that correspond to only 2 physical cores. Slurm apparently counts the physical cores, and so we can't request more than 2.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The relationship between the physical and virtual cores makes sense, and it also makes me wonder if we might be able to programmatically get the CPUs per task from the state of the environment somehow so that if we change the VM class, we don't also have to remember to update this module-level variable. Is that something we can easily get from the compute environment?

json_path = f'{workspace_dir}/datastack/parameters.invest.json'
return textwrap.dedent(f"""\
#!/bin/sh
#SBATCH --cpus-per-task={CPUS_PER_TASK}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

By default --cpus-per-task is 1, hence why tasks were not being parallelized.

region = var.region
reserved_ip_range = module.private_service_access.reserved_ip_range
zone = var.zone
size_gb = 2560

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Explicitly request the minimum filestore size, 2560 GB.

@emlys emlys changed the title Bugfix/33 Allow >1 CPU per task, and some misc clean up Jun 1, 2026
@emlys
emlys requested a review from phargogh June 1, 2026 20:41
@emlys
emlys marked this pull request as ready for review June 1, 2026 20:42
@emlys emlys self-assigned this Jun 1, 2026

@phargogh phargogh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @emlys ! I just had the one comment about possibly getting the number of CPUs per task from the environment, just to save us effort down the road. No worries if it isn't easy to extract that information, but it might be handy if we can!

}
}

CPUS_PER_TASK = 2 # number of cpus per task to submit to sbatch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The relationship between the physical and virtual cores makes sense, and it also makes me wonder if we might be able to programmatically get the CPUs per task from the state of the environment somehow so that if we change the VM class, we don't also have to remember to update this module-level variable. Is that something we can easily get from the compute environment?

@emlys

emlys commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@phargogh good point! I looked into this and I think we can achieve the same thing by setting --exclusive, rather than requesting the max number of CPUs. --exclusive tells slurm to give this job exclusive access to an entire node. My understanding is that by default, CPUs are assigned to only one job at a time (and sharing a CPU between multiple jobs would be detrimental for CPU-bound tasks). And we're not using any GPUs, so a job that uses the max number of CPUs effectively claims the entire node.

We may want to revisit this if we upgrade the compute nodes, or for a demo server we might want to limit each job to 1 CPU.

Let's ignore the failing checks for now, I made some changes on another branch that messed up the authentication from Github Actions to the storage bucket. Should be fixed in the next PR.

@emlys
emlys requested a review from phargogh June 4, 2026 18:13

@phargogh phargogh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, simple solution! I like the --exclusive flag. Thanks for humoring my question!

@phargogh
phargogh merged commit f317037 into main Jun 4, 2026
0 of 3 checks passed
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.

invest processes are all sharing one core

2 participants