-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat: add PyHPS CLI #3091
feat: add PyHPS CLI #3091
Conversation
It works
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3091 +/- ##
==========================================
- Coverage 87.12% 80.68% -6.44%
==========================================
Files 55 60 +5
Lines 9768 10556 +788
==========================================
+ Hits 8510 8517 +7
- Misses 1258 2039 +781 |
src/ansys/mapdl/core/cli/hpc.py
Outdated
) | ||
|
||
if config_file is None: | ||
config_file = os.path.join(os.getcwd(), "hps_config.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@germa89 should the config be in appdirs
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MMhhh... if we use a file in the root directory, we could point to different files.... and hence multiple configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe you are right... maybe it is better to use appdirs
.... buttt... it might be a bit hidden for the user...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we have many options to be keep in the file:
{
"url": "https://10.231.106.91:3000/hps",
"user": "repuser",
"password": "repuser",
"python": "3.9",
"name": "my job",
"num_cores": 1,
"memory": 100,
"disk_space": 100,
"exclusive": false,
"max_execution_time": 1000
}
keeping so many important options hidden in a appdir
path might bring some complications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@germa89 use keyring for user/password. I don't think its secure enough to put that in plaintext like this. For everything else I would agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
src/ansys/mapdl/core/hpc/pyhps.py
Outdated
requirements_file = create_tmp_file("requirements.txt", content) | ||
logger.debug(f"Requirements file in: {requirements_file}") | ||
|
||
if not shell_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shell file is needed because pyhps doesn't support python jobs with a venv. I think we should see if that feature can be added to pyhps itself. Maybe using pip-run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we need to support venv within pyhps. Using pip-run
seems interesting. For flexibility I would keep an optional shell file though.
src/ansys/mapdl/core/cli/hpc.py
Outdated
requirements_file: str = None, | ||
extra_files: Optional[Union[str, list]] = None, | ||
config_file: str = None, | ||
num_cores: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option to download the result files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware if we can do that from the PyHPS library... i will check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It can be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* First approach to login * Adding HPS dependencies * Adding changelog entry: 3205.miscellaneous.md * feat: Coupling login code to current implementation. Allowing login using token which is now the preferred method. * coupling cli * fix: command name in CLI * Adding changelog entry: 3205.added.md * fix: wrong argument that avoid having the input file as argument * chore: checking config file in submit function. * feat: avoid venv creation of ``requirements_file`` is False. * feat: login CLI finished * feat: making sure we don't get import errors when PyHPS is not installed. * feat: Adding docstrings * chore: renaming 'access' to 'get_token_access'. * fix: failing piping on the CLI. --------- Co-authored-by: pyansys-ci-bot <pyansys.github.bot@ansys.com>
for more information, see https://pre-commit.ci
if not quiet: | ||
click.echo("Token has been verified with the HPC cluster.") | ||
|
||
logger.info(f"Stored credentials: {user}, {password}, {url}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that sensitive information such as passwords is not logged. Instead of logging the password, we can log a placeholder or omit it entirely from the log message. This change should be made on line 172 where the sensitive information is being logged.
The best way to fix this without changing existing functionality is to modify the log message to exclude the password. We can log the user and URL while omitting the password. This change will be made in the login
function in the file src/ansys/mapdl/core/cli/login.py
.
-
Copy modified line R172
@@ -171,3 +171,3 @@ | ||
|
||
logger.info(f"Stored credentials: {user}, {password}, {url}") | ||
logger.info(f"Stored credentials: {user}, [REDACTED], {url}") | ||
store_credentials(user, password, url, default=default) |
} | ||
|
||
logger.debug( | ||
f"Saving the following configuration to the config file ({config_file}):\n{config}." |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that sensitive information such as passwords is not logged in clear text. The best way to fix this without changing existing functionality is to remove the password from the config
dictionary before logging it. This way, the configuration can still be logged for debugging purposes without exposing sensitive information.
We will modify the code in the submit
function to create a copy of the config
dictionary without the password before logging it. This change will be made around line 364 in the file src/ansys/mapdl/core/cli/submit.py
.
-
Copy modified lines R363-R364 -
Copy modified line R366
@@ -362,4 +362,6 @@ | ||
|
||
config_to_log = config.copy() | ||
config_to_log.pop("password", None) | ||
logger.debug( | ||
f"Saving the following configuration to the config file ({config_file}):\n{config}." | ||
f"Saving the following configuration to the config file ({config_file}):\n{config_to_log}." | ||
) |
|
||
|
||
def set_password(*args, **kwargs): | ||
logger.debug(f"Setting password from service '{args[0]}' and key '{args[1]}'") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to remove or modify the logging statements that include sensitive information such as passwords. Instead of logging the actual password, we can log a generic message indicating that a password operation is being performed without including the sensitive data. This way, we maintain the logging functionality for debugging purposes without exposing sensitive information.
Specifically, we need to:
- Modify the logging statements in the
set_password
function to avoid logging the actual password. - Modify the logging statement in the
login_in_cluster
function to avoid logging the actual password.
-
Copy modified line R51 -
Copy modified line R73
@@ -50,3 +50,3 @@ | ||
def set_password(*args, **kwargs): | ||
logger.debug(f"Setting password from service '{args[0]}' and key '{args[1]}'") | ||
logger.debug(f"Setting password for service '{args[0]}'") | ||
return keyring.set_password(*args, **kwargs) | ||
@@ -72,3 +72,3 @@ | ||
""" | ||
logger.debug(f"Authenticating on cluster '{url}' using user '{user}'.") | ||
logger.debug(f"Authenticating on cluster '{url}' using user '{user}' with a provided password.") | ||
access_token = authenticate( |
str | ||
Access token. | ||
""" | ||
logger.debug(f"Authenticating on cluster '{url}' using user '{user}'.") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we should avoid logging sensitive information such as URLs and usernames. Instead, we can log a generic message that does not include these details. This way, we maintain the logging functionality without exposing sensitive data.
- Replace the log message on line 73 with a generic message that does not include the URL and username.
- Ensure that no sensitive information is logged in other parts of the code.
-
Copy modified line R73
@@ -72,3 +72,3 @@ | ||
""" | ||
logger.debug(f"Authenticating on cluster '{url}' using user '{user}'.") | ||
logger.debug("Authenticating on cluster.") | ||
access_token = authenticate( |
expiration_time = float(expiration_time) | ||
|
||
logger.debug( | ||
f"Retrieved info for '{identifier}': {url}, {user}, {password}, {timestamp}, {expiration_time} " |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
This expression logs
sensitive data (password)
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that sensitive information such as URLs, usernames, and passwords are not logged in clear text. Instead, we can log non-sensitive information or mask the sensitive parts of the data. Specifically, we should modify the log message on line 155 to exclude or mask the sensitive information.
The best way to fix this without changing existing functionality is to log only the identifier and a message indicating that credentials were retrieved, without including the actual sensitive data. This can be done by updating the log message to exclude the URL, username, password, timestamp, and expiration time.
-
Copy modified line R155
@@ -154,3 +154,3 @@ | ||
logger.debug( | ||
f"Retrieved info for '{identifier}': {url}, {user}, {password}, {timestamp}, {expiration_time} " | ||
f"Retrieved credentials for '{identifier}'" | ||
) |
As the title.
The idea is you can have:
$ pymapdl submit main_file.py $ pymapdl submit --name="my project" main_file.py --requirements_file=requirements.txt
The help shows: