-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
An alternate option: But I don't think that this immediately removes the file. |
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. |
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.
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.
let's just create a temporary folder, and put all outputs and log files there. |
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 |
@@ -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" |
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.
is there any other fixture or test function that requires the new output directory?
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.
what about this hardcoded log path?
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.
bump for hardcoded path?
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.
bump for hardcoded path?
|
||
|
||
@pytest.fixture(scope="module") | ||
def module_file_path_f(request, tmpdir): |
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.
are these necessary? each function can just use os.path.join and add a prefix, no?
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.
bump
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.
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.
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.
can't we just have a single session-scope fixture to set a single temp folder, and then dump all artifacts there?
tests/test_features.py
Outdated
else: | ||
pytest.skip(f"not tested for tokamak {tokamak.value}") | ||
|
||
while not isinstance(test_setting, ShotSettings): |
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.
why the while? isn't this function parameterized so that it only gets one element at a time?
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.
bump
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.
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.
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.
The more that I think about it though, this might be the way
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 still don't get it, could you explain it again with different wording?
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.
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.
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.
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.
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'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.
Nice changes, chan this be merged now? |
there are a couple of minor comments, try opening the PR on the website! |
I went a bit haywire and fixed a bunch of fundamental structural issues with the |
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 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 |
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.
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: |
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.
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" |
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.
bump for hardcoded path?
disruption_py/utils/exceptions.py
Outdated
from disruption_py.utils.mappings.tokamak import Tokamak | ||
|
||
|
||
class TokamakNotSupportedError(Exception): |
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.
do we need a new class?
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.
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.
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.
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): |
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.
bump
tests/test_features.py
Outdated
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"))]): |
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.
IMHO there must be a better way!
tests/test_features.py
Outdated
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"))]): |
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.
hmm... same.
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. |
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.
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" |
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.
bump for hardcoded path?
disruption_py/utils/exceptions.py
Outdated
from disruption_py.utils.mappings.tokamak import Tokamak | ||
|
||
|
||
class TokamakNotSupportedError(Exception): |
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.
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): |
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.
can't we just have a single session-scope fixture to set a single temp folder, and then dump all artifacts there?
Temporary file implementation for testing.
fix #145 .