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

Initial version of the configuration autodetection script #240

Open
wants to merge 39 commits into
base: alps
Choose a base branch
from

Conversation

Blanca-Fuentes
Copy link

@Blanca-Fuentes Blanca-Fuentes commented Oct 17, 2024

Automatic detection of the system configuration to create the reframe configuration file. User input is required to generate the final reframe config_file for the system: <system_name>_config.py and <system_name>_config.json.

Copy link
Collaborator

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

Some very minor first comments

config/generate_json.py Outdated Show resolved Hide resolved
config/generate_json.py Outdated Show resolved Hide resolved
config/generate_json.py Outdated Show resolved Hide resolved
config/generate_json.py Outdated Show resolved Hide resolved
config/generate_json.py Outdated Show resolved Hide resolved
@ekouts ekouts marked this pull request as ready for review October 23, 2024 15:20
@ekouts ekouts requested review from jgphpc and teojgo October 23, 2024 15:20
Copy link
Collaborator

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

Nice job! Just a couple of initial comments, I will try more tomorrow

config/reframe_config_template.j2 Outdated Show resolved Hide resolved
config/utils.py Outdated Show resolved Hide resolved
config/generate_config.py Outdated Show resolved Hide resolved
config/generate_config.py Outdated Show resolved Hide resolved
config/generate_config.py Outdated Show resolved Hide resolved
config/generate_config.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@teojgo teojgo left a comment

Choose a reason for hiding this comment

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

I added only some minor things that I have seen. I am now going through the testing o f the functionality.

config/README.md Outdated Show resolved Hide resolved
config/README.md Outdated Show resolved Hide resolved
config/reframe_config_template.j2 Outdated Show resolved Hide resolved
config/generate_config.py Outdated Show resolved Hide resolved
@jgphpc
Copy link
Collaborator

jgphpc commented Nov 8, 2024

cscs-ci run alps-todi-cpe

@jgphpc
Copy link
Collaborator

jgphpc commented Nov 8, 2024

cscs-ci run alps-todi-uenv

1 similar comment
@jgphpc
Copy link
Collaborator

jgphpc commented Nov 8, 2024

cscs-ci run alps-todi-uenv

Copy link
Collaborator

@ekouts ekouts left a comment

Choose a reason for hiding this comment

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

Overall it looks good. I have a few more comments before approving.

config/reframe_config_template.j2 Show resolved Hide resolved
config/utilities/job_util.py Outdated Show resolved Hide resolved
@ekouts ekouts requested a review from teojgo November 12, 2024 12:28
UNDERLINE = '\033[4m'


RFM_DOCUMENTATION = {'modules': 'https://reframe-hpc.readthedocs.io/en/stable/config_reference.html#config.systems.modules',
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of repeating https://..., can you use a variable ?
something like:

url = 'https://reframe-hpc.readthedocs.io/en/stable/'
...
'modules' = f'{url}/config_reference.html#config.systems.modules',

fi

if check_lmod; then
# Check if it's available as a module, regardless of 'which' result
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Check if it's available as a module, regardless of 'which' result
# Check if it is available as a module, regardless of 'which' result


# Output the total memory
echo "Total memory: $total_memory"
'''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'''
'''

import subprocess


class ModulesSystem(abc.ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

ABC ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

abstract base class

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.

4 participants