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

Jlorincz/temp files + shot settings from dict #154

Merged
merged 18 commits into from
Jun 13, 2024
Merged

Conversation

lajz
Copy link
Collaborator

@lajz lajz commented May 18, 2024

Temporary file implementation for testing.

fix #145 .

@lajz
Copy link
Collaborator Author

lajz commented May 18, 2024

An alternate option:
https://docs.pytest.org/en/6.2.x/tmpdir.html

But I don't think that this immediately removes the file.

@lajz
Copy link
Collaborator Author

lajz commented May 18, 2024

Also I changed the log file removal to remove before testing starts instead of after testing ends, so that the log file can actually be used. I can change it back if there is some issue caused by this.

@gtrevisan gtrevisan changed the base branch from main to dev May 20, 2024 13:18
Copy link
Member

@gtrevisan gtrevisan left a comment

Choose a reason for hiding this comment

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

please either create a few NamedTemporaryFile objects or a single TemporaryDirectory.
then cleanup_after_tests can easily purge the folder or files.

watch out that __file__ does contain a path, not just the basename!

logfiles if stored in a temporary folder can be even left there for the system to cleanup.

@gtrevisan
Copy link
Member

let's just create a temporary folder, and put all outputs and log files there.
please don't clean that up as it might be useful to check logs after the execution is completed.
in any case, those are all small files.

lajz added 4 commits May 31, 2024 07:34
@lajz
Copy link
Collaborator Author

lajz commented May 31, 2024

I think that this is better, for the future we will need to make the base of the temp folder inside of a scratch directory

.gitignore Outdated Show resolved Hide resolved
@@ -15,7 +15,9 @@
from disruption_py.utils.math_utils import matlab_gradient_1d_vectorized


def get_mdsplus_data(handler: Handler, shot_ids: List[int]) -> Dict[int, pd.DataFrame]:
def get_mdsplus_data(
handler: Handler, shot_ids: List[int], log_file_path="tests/eval_against_sql.log"
Copy link
Member

Choose a reason for hiding this comment

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

is there any other fixture or test function that requires the new output directory?

Copy link
Member

Choose a reason for hiding this comment

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

what about this hardcoded log path?

Copy link
Member

@gtrevisan gtrevisan Jun 11, 2024

Choose a reason for hiding this comment

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

bump for hardcoded path?

Copy link
Member

Choose a reason for hiding this comment

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

bump for hardcoded path?

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved


@pytest.fixture(scope="module")
def module_file_path_f(request, tmpdir):
Copy link
Member

Choose a reason for hiding this comment

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

are these necessary? each function can just use os.path.join and add a prefix, no?

Copy link
Member

Choose a reason for hiding this comment

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

bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this is needed because of pytest scope issues. If you use the scope="file" fixture for everything it breaks when used in a module level scope.

Copy link
Member

Choose a reason for hiding this comment

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

can't we just have a single session-scope fixture to set a single temp folder, and then dump all artifacts there?

else:
pytest.skip(f"not tested for tokamak {tokamak.value}")

while not isinstance(test_setting, ShotSettings):
Copy link
Member

Choose a reason for hiding this comment

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

why the while? isn't this function parameterized so that it only gets one element at a time?

Copy link
Member

Choose a reason for hiding this comment

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

bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is to handle the case of a function and a dictionary in any order. I don't really like that the function part exists at all as it will make it much harder to put in settings, but removing it will require formatting strings and be hard to adapt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The more that I think about it though, this might be the way

Copy link
Member

Choose a reason for hiding this comment

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

I still don't get it, could you explain it again with different wording?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically we need two pieces of information at runtime to determine the shot settings that we would like to use. The first is what tokamak we are using (this will no longer be the case once we remove the handler classes), the second is the temp directory. The tokamak is currently handled via the option for a dictionary as an entry in the dictionary and the temp file path is passed via a lambda function. The problem is that it is hard to pass the temp file path without this lambda method.

Copy link
Collaborator Author

@lajz lajz Jun 11, 2024

Choose a reason for hiding this comment

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

Thinking about it again, I think that the best solution is to use an environment variable as the base path for logging. Then we do not need any of these lambda methods that complicates things. This would also help us force users to use certain log paths on different servers.

Copy link
Member

@gtrevisan gtrevisan left a comment

Choose a reason for hiding this comment

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

I'm very confused, we can talk about this briefly -- isn't this overengineered? just create a unique folder under /tmp for the tests and call it a day, each function can choose a file name with extension within there... even a temp file works! you'd just need to print its location in the output.

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
lajz and others added 4 commits June 7, 2024 14:57
@lajz
Copy link
Collaborator Author

lajz commented Jun 10, 2024

Nice changes, chan this be merged now?

@gtrevisan
Copy link
Member

there are a couple of minor comments, try opening the PR on the website!

@lajz
Copy link
Collaborator Author

lajz commented Jun 11, 2024

I went a bit haywire and fixed a bunch of fundamental structural issues with the test_features test that should also help with future implementations of running from configs. The PR will need another review though @gtrevisan

Copy link
Member

@gtrevisan gtrevisan left a comment

Choose a reason for hiding this comment

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

I think we need to chat about this again! 🚧

@@ -29,6 +34,7 @@ class LogSettings:
Whether to use custom logging. If set to true, no logging setup will be done. Default is False.
"""

log_to_file: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

do we need this? it seems to me the log file path is sufficient, None means off, path means on.

@@ -59,8 +65,18 @@ def setup_logging(self, logger_name="disruption_py"):
logger.propagate = False
logger.handlers.clear()
logger.setLevel(logging.DEBUG)
if self.log_file_path is not None:
fh = logging.FileHandler(self.log_file_path, mode=self.log_file_write_mode)
if self.log_to_file or self.log_file_path is not None:
Copy link
Member

Choose a reason for hiding this comment

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

does this apply to all logging, or just test logging?
nobody will choose a log file path as an env variable.

@@ -15,7 +15,9 @@
from disruption_py.utils.math_utils import matlab_gradient_1d_vectorized


def get_mdsplus_data(handler: Handler, shot_ids: List[int]) -> Dict[int, pd.DataFrame]:
def get_mdsplus_data(
handler: Handler, shot_ids: List[int], log_file_path="tests/eval_against_sql.log"
Copy link
Member

@gtrevisan gtrevisan Jun 11, 2024

Choose a reason for hiding this comment

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

bump for hardcoded path?

disruption_py/settings/shot_settings.py Show resolved Hide resolved
from disruption_py.utils.mappings.tokamak import Tokamak


class TokamakNotSupportedError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

do we need a new class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a much better practice, at least; I think there should be a future PR where we add this exception elsewhere for unsupported tokamak issues and create a few more exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

can you remove this to keep it simple?
we will evaluate any custom exceptions, if needed, in a separate PR.

explicit and informative error messages are of paramount importance, not custom exceptions (for now)! thanks

tests/conftest.py Show resolved Hide resolved


@pytest.fixture(scope="module")
def module_file_path_f(request, tmpdir):
Copy link
Member

Choose a reason for hiding this comment

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

bump

tests/test_features.py Show resolved Hide resolved
if tokamak.value in test_setting:
test_setting = test_setting[tokamak.value]
else:
with temporary_env_vars([("FILE_LOG_PATH", test_file_path_f(".log"))]):
Copy link
Member

Choose a reason for hiding this comment

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

IMHO there must be a better way!

num_processes=2,
)
def test_features_parallel(handler: Handler, tokamak, shotlist, test_file_path_f):
with temporary_env_vars([("FILE_LOG_PATH", test_file_path_f(".log"))]):
Copy link
Member

Choose a reason for hiding this comment

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

hmm... same.

lajz added 3 commits June 11, 2024 17:42
@lajz lajz changed the title Jlorincz/temp files Jlorincz/temp files + shot settings from dict Jun 11, 2024
@lajz
Copy link
Collaborator Author

lajz commented Jun 11, 2024

It is in a cleaner state now. As a note, the temp file and shot settings from dict could be separate PRs, but now that they are together, I do not think that it is worth separating them.

Copy link
Member

@gtrevisan gtrevisan left a comment

Choose a reason for hiding this comment

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

there is one lingering hardcoded path, please see comments.
and let's keep it clean of custom exceptions for now.

then we can merge!

@@ -15,7 +15,9 @@
from disruption_py.utils.math_utils import matlab_gradient_1d_vectorized


def get_mdsplus_data(handler: Handler, shot_ids: List[int]) -> Dict[int, pd.DataFrame]:
def get_mdsplus_data(
handler: Handler, shot_ids: List[int], log_file_path="tests/eval_against_sql.log"
Copy link
Member

Choose a reason for hiding this comment

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

bump for hardcoded path?

from disruption_py.utils.mappings.tokamak import Tokamak


class TokamakNotSupportedError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

can you remove this to keep it simple?
we will evaluate any custom exceptions, if needed, in a separate PR.

explicit and informative error messages are of paramount importance, not custom exceptions (for now)! thanks



@pytest.fixture(scope="module")
def module_file_path_f(request, tmpdir):
Copy link
Member

Choose a reason for hiding this comment

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

can't we just have a single session-scope fixture to set a single temp folder, and then dump all artifacts there?

@lajz lajz requested a review from gtrevisan June 12, 2024 22:25
@gtrevisan gtrevisan merged commit 81d01a0 into dev Jun 13, 2024
6 checks passed
@gtrevisan gtrevisan deleted the jlorincz/temp_files branch June 13, 2024 12:30
This was referenced Jun 25, 2024
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.

None yet

2 participants